Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASAN: invalid handling of scanf("%mc") format option #61768

Closed
nalajcie opened this issue Mar 28, 2023 · 4 comments · Fixed by llvm/llvm-project-release-prs#658
Closed

ASAN: invalid handling of scanf("%mc") format option #61768

nalajcie opened this issue Mar 28, 2023 · 4 comments · Fixed by llvm/llvm-project-release-prs#658

Comments

@nalajcie
Copy link

Test case with false positive:

#include <stdio.h>
#include <stdlib.h>

int main(int argc, char **argv)
{
    const char *buff = "foo";
    char *res = NULL;

    // ASAN fails inside scanf
    sscanf(buff, "%mc", &res);

    free(res);
    return 0;
}

The above code fails under glibc-2.35 (but the responsible code wasn't changed in the last 9 years).

Responsible code: https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors_format.inc#L347

IMHO it's invalid to assume the NULL character would be appended in case of %[precision]mc so strlen can't be used here (and +1 is strictly invalid). POSIX-2008:TC2 standard is kind of ambiguous about that, but I've found discussion about changes going into TC3 (when it would be published) which explicitly states that NULL character would be appended in s case only: https://www.austingroupbugs.net/view.php?id=1173

Other standard libraries aside of glibc were not inspected.

@EugeneZelenko EugeneZelenko added compiler-rt:asan Address sanitizer false-positive Warning fires when it should not and removed new issue labels Mar 28, 2023
@MaskRay
Copy link
Member

MaskRay commented Aug 22, 2023

Thanks for the detailed report. The write range for %mC and %mS are incorrect as well.

Patch: https://reviews.llvm.org/D158485

@MaskRay MaskRay self-assigned this Aug 22, 2023
@MaskRay MaskRay added this to the LLVM 17.0.X Release milestone Aug 28, 2023
@MaskRay
Copy link
Member

MaskRay commented Aug 28, 2023

/cherry-pick beeb37a

@MaskRay MaskRay reopened this Aug 28, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 28, 2023

/branch llvm/llvm-project-release-prs/issue61768

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 28, 2023
When the optional assignment-allocation character 'm' (Extension to the
ISO C standard) is present, we currently use internal_strlen(buf)+1 for
all of cCsS[ (D85350). Fix cCS to use the correct size.

Fix llvm/llvm-project#61768

Reviewed By: #sanitizers, vitalybuka

Differential Revision: https://reviews.llvm.org/D158485

(cherry picked from commit beeb37a8f3275281be305d2d1afe35ca053e21c0)
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 28, 2023

/pull-request llvm/llvm-project-release-prs#658

tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Sep 5, 2023
When the optional assignment-allocation character 'm' (Extension to the
ISO C standard) is present, we currently use internal_strlen(buf)+1 for
all of cCsS[ (D85350). Fix cCS to use the correct size.

Fix llvm/llvm-project#61768

Reviewed By: #sanitizers, vitalybuka

Differential Revision: https://reviews.llvm.org/D158485

(cherry picked from commit beeb37a8f3275281be305d2d1afe35ca053e21c0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

4 participants