Skip to content

cpython3:fuzz_builtin_unicode: Use-of-uninitialized-value in maybe_small_long #102509

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

Closed
illia-v opened this issue Mar 7, 2023 · 11 comments
Closed
Labels
type-bug An unexpected behavior, bug, or error

Comments

@illia-v
Copy link
Contributor

illia-v commented Mar 7, 2023

There is a bug disclosed by oss-fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=51574.

I can reproduce it by running CC=clang ./configure --with-memory-sanitizer && make -j12.

Linked PRs

@illia-v illia-v added the type-bug An unexpected behavior, bug, or error label Mar 7, 2023
illia-v added a commit to illia-v/cpython that referenced this issue Mar 7, 2023
@terryjreedy
Copy link
Member

terryjreedy commented Mar 7, 2023

@ammaraskar commented on the report, asking if it has been 'triaged'. There was no response; it appears from text on the report is that it is not monitored by people on the project. The report claims "Issue filed automatically" (without a URL) but I found no evidence searching cpython open/closed issues for 'unicode' or 'uninitialized'.

@illia-v
Copy link
Contributor Author

illia-v commented Mar 8, 2023

@terryjreedy I think "Issue filed automatically" refers to the issue 51574 on https://bugs.chromium.org/p/oss-fuzz/issues/list,

@ammaraskar
Copy link
Member

Terry when I was asking if it was triaged there I was referring to whether the other core devs who have access to the fuzzer results had taken a look and filed a CPython issue.

illia-v added a commit to illia-v/cpython that referenced this issue Mar 13, 2023
@mdickinson
Copy link
Member

See comments on the PR; I believe this is a false positive, and can safely be closed.

@mdickinson mdickinson added the pending The issue will be closed if no feedback is provided label Mar 18, 2023
@mdickinson
Copy link
Member

tl;dr: In the case in question, we do retrieve an integer from allocated but uninitialised memory, but we then multiply that integer by zero, so the lack of initialisation has no adverse effect.

@illia-v
Copy link
Contributor Author

illia-v commented Mar 18, 2023

@mdickinson thanks for looking into this! I assumed it is a false positive, but it prevents fuzzing and building --with-memory-sanitizer.

Can #102510 be accepted to fix the issues?
If not, the --with-memory-sanitizer flag should let a compiler know that it is a known issue probably. https://clang.llvm.org/docs/SanitizerSpecialCaseList.html

@mdickinson
Copy link
Member

but it prevents fuzzing and building --with-memory-sanitizer.

Hmm, that's awkward. Is there no way to annotate to tell the sanitiser that this case has been checked and deemed safe? E.g., something like this: https://clang.llvm.org/docs/SanitizerSpecialCaseList.html

Slowing down a hot path by adding an unnecessary extra step just so that a tool doesn't emit a false positive error doesn't seem like the right solution.

@illia-v
Copy link
Contributor Author

illia-v commented Mar 20, 2023

Hmm, that's awkward. Is there no way to annotate to tell the sanitiser that this case has been checked and deemed safe? E.g., something like this: https://clang.llvm.org/docs/SanitizerSpecialCaseList.html

Yes, ignoring is an option, I mentioned it in my previous comment 🙂

#102838, I checked and it looks to fix python infra/helper.py reproduce cpython3 fuzz_builtin_unicode clusterfuzz-testcase-minimized-fuzz_builtin_unicode-6313066228219904.

@markshannon
Copy link
Member

The C spec says that an uninitialized value can include trap values, so in theory multiplying by zero could trap.
That won't happen on any mainstream hardware, but we should probably fix it.

@AlexWaygood AlexWaygood removed the pending The issue will be closed if no feedback is provided label Jul 28, 2023
illia-v added a commit to illia-v/cpython that referenced this issue Jul 28, 2023
@illia-v
Copy link
Contributor Author

illia-v commented Jul 28, 2023

@markshannon could you please check if #102510 is an acceptable fix?

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 30, 2023
Yhg1s pushed a commit that referenced this issue Jul 31, 2023
…-102510) (#107464)

gh-102509: Start initializing `ob_digit` of `_PyLongValue` (GH-102510)
(cherry picked from commit fc130c4)

Co-authored-by: Illia Volochii <[email protected]>
@illia-v
Copy link
Contributor Author

illia-v commented Aug 2, 2023

I'm closing this issue because the fix has been verified by ClusterFuzz. Thanks everyone involved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

6 participants