Skip to content

Use of uninitialized memory in Objects/longobject.c #106914

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
Yhg1s opened this issue Jul 20, 2023 · 4 comments
Closed

Use of uninitialized memory in Objects/longobject.c #106914

Yhg1s opened this issue Jul 20, 2023 · 4 comments
Assignees
Labels
type-bug An unexpected behavior, bug, or error

Comments

@Yhg1s
Copy link
Member

Yhg1s commented Jul 20, 2023

When using MemorySanitizer, maybe_small_long() in Objects/longobject.c is flagged as using uninitialized memory when passed certain values, which means Python main, 312 and 3.11 can't even be built correctly with --with-memorysanitizer:

% CC=clang-13 CXX=clang++-13 ./configure --with-memory-sanitizer
[...]
% make
[...]
./_bootstrap_python Programs/_freeze_module.py abc Lib/abc.py Python/frozen_modules/abc.h
==21198==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7c5058 in maybe_small_long Objects/longobject.c:62:13
    #1 0x7c5058 in long_bitwise Objects/longobject.c:5267:24
    #2 0x7b81a6 in long_and Objects/longobject.c:5279:12
[...]
  Uninitialized value was created by a heap allocation
    #0 0x45288d in __interceptor_malloc (_bootstrap_python+0x45288d)
    #1 0x878da6 in _PyMem_RawMalloc Objects/obmalloc.c:45:12
    #2 0x87bb6f in PyObject_Malloc Objects/obmalloc.c:801:12
    #3 0x7bf9fc in _PyLong_New Objects/longobject.c:158:14
    #4 0x7bf9fc in long_bitwise Objects/longobject.c:5225:9
    #5 0x7b81a6 in long_and Objects/longobject.c:5279:12
[...]
SUMMARY: MemorySanitizer: use-of-uninitialized-value Objects/longobject.c:62:13 in maybe_small_long
Exiting
make: *** [Makefile:1332: Python/frozen_modules/abc.h] Error 1

The source of the problem is _PyLong_CompactValue (from Include/cpython/longintrepr.h) multiplying ob_digit[0] by size, where ob_digit[0] may be uninitialized when size is 0 (the simplest reproducer for that is int('0')). (In Python 3.11 this same logic happens in medium_value() in longobject.c instead. Python 3.10 had similar logic in the MEDIUJM_VALUE macro in longobject.c, but it had an explicit check for size being 0.)

(I believe, but I'm not entirely sure, the code relies on undefined behaviour, which we should avoid -- although it's only undefined for very esoteric reasons. Even so, the simple solution is to add back the check for sign being 0. If that really has a noticeable impact, we could add the branch only when using MemorySanitizer.)

I believe Greg was working on getting us to a point where we could have a MSan buildbot (#79395), although I don't know what the state of that is now.

@Yhg1s Yhg1s added the type-bug An unexpected behavior, bug, or error label Jul 20, 2023
@AlexWaygood
Copy link
Member

Duplicate of #102509?

@gpshead
Copy link
Member

gpshead commented Jul 20, 2023

I believe Greg was working on getting us to a point where we could have a MSan buildbot (#79395), although I don't know what the state of that is now.

I started down that path at one point in 2018-19ish... but found MSAN so painful to setup as it really doesn't work well in an environment where you haven't compiled the entire system (all transitive dependency dynamically linked libraries) with MSAN. Without that it relied on an MSAN baked in exclusion list of known "misbehaviors" from various libraries otherwise and that list vs actual implementation was never sufficient for results I'd want to surface in front of people. We should revisit this at some point, maybe the feasibility has changed? (internally at work where we build our entire stack, MSAN works pretty well, because a team exists to keep it relevant)

Regardless, this may well be a dup as Alex pointed out (thanks!). Investigation required; there's some interesting discussion over there.

@markshannon
Copy link
Member

Duplicate of #102509?

Yes, closing.

@markshannon markshannon closed this as not planned Won't fix, can't repro, duplicate, stale Jul 28, 2023
@illia-v
Copy link
Contributor

illia-v commented Aug 5, 2023

@gpshead the MSAN tests can be set up using CIFuzz pretty cleanly, please check #107653

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

5 participants