Raise where the fuzzer found aborts and stack overflows, and collect with the count in the object header - #8551
Draft
youknowone wants to merge 12 commits into
Draft
Raise where the fuzzer found aborts and stack overflows, and collect with the count in the object header#8551youknowone wants to merge 12 commits into
youknowone wants to merge 12 commits into
Conversation
The comment said non-unix threading builds have no stop-the-world. CollectStopTheWorld is gated on `feature = "threading"` alone, and sys._current_frames stops the world on both paths; the field is the fallback a reader uses when there is no `top_iframe` to materialize from. Assisted-by: Claude
`SetFilePointer` answers with the low half of the new position and signals failure with INVALID_SET_FILE_POINTER, which is also that half of a position four gigabytes in; telling them apart takes the error code, which this did not read, so such a seek was reported as an error. Deciding seekability from it also called such a file unseekable. `SetFilePointerEx` returns the whole position and a success flag of its own, which also removes the transmute of the position into halves. Assisted-by: Claude
Assigning an item reported every packing failure as a TypeError, so
m[0] = 300 on a 'B' view said the value was the wrong type rather than out
of range. Packing now says which of the two it was, and whether the value's
own code raised, in which case that error is the answer as it is:
m[0] = 300 ValueError: invalid value for format 'B'
m[0] = "x" TypeError: invalid type for format 'B'
m[0] = <__index__ that raises> the raised error
`struct` reports both as `struct.error` and is unchanged; the kind travels
beside the exception for the caller that tells them apart.
Also: None was read as a deletion, so m[0] = None answered "cannot delete
memory" instead of packing it; and deleting through the mapping protocol
never reached the read-only check, which comes first.
Assisted-by: Claude
os.read, _RawIOBase.read, int.to_bytes, struct.pack, ctypes array creation and the _ssl RAND functions sized a Vec from a Python-supplied length with vec![], which calls handle_alloc_error and, under panic = "abort", ends the process. They now allocate through vm.new_zeroed_bytes and raise MemoryError. itertools.product built its pools without checking that len(iterables) * repeat is representable; it now raises OverflowError "repeat argument too large" and reserves the pool and index vectors fallibly. repeat is read as isize, so a negative one raises ValueError "repeat argument cannot be negative" instead of the conversion's message. _ssl.RAND_bytes and RAND_pseudo_bytes read n as i32, matching the int they are declared with. Assisted-by: Claude
A collection kept the count it was working with in a table keyed by the object's address, and the objects it had proved reachable in a second one. Between them they were hashed once per candidate and twice per edge in the heap, which is where most of a collection over a live heap went. The count now lives in `PyInner::gc_refs`, with `GcBits::COLLECTING` saying it is meaningful, and reachability is `gc_refs == GC_REACHABLE` rather than membership in a set. Step 5 splits the candidates and clears the bit in one pass. gcbench, five interleaved pairs, median: a live heap of 423k objects goes from 0.101s to 0.055s and a dead one from 0.402s to 0.383s. The bits, generation, owner and count take eight bytes between them. A 64-bit header had those eight as the padding its alignment forces, so it is unchanged at 48 bytes; a 32-bit header grows from 24 to 28. Assisted-by: Claude
A debug build already started Python threads on 8 MiB rather than Rust's 2 MiB default, but an explicit threading.stack_size(N) went through verbatim. test_threading asks for 256 KiB, and starting a thread on that walked off the end of the stack: the guard page fault landed in the prologue of ExecutingFrame::run. Unoptimized, that prologue reserves 80,848 bytes where the optimized one reserves 656 -- execute_instruction is #[inline(always)] and LLVM only colors stack slots from opt-level 1, so the frame is the sum of all 200 instruction arms' temporaries rather than the largest. A Python call costs 88,672 bytes of native stack there, and threading's bootstrap is six frames deep, so 256 KiB holds less than half of what starting a thread takes. The floor reaches thread::Builder only; threading.stack_size() still answers with what was asked for, and release builds are unchanged. Assisted-by: Claude
The C-stack guard ran on one frame entry in eight. That asks the margin to cover eight frames rather than one, and it does not: an unoptimized frame entered through native code takes 88,672 bytes against a debug margin of 262,144. A recursion whose steps re-enter that way -- `__add__` calling itself, a sort key that sorts -- ran off the end of the stack instead of raising RecursionError. On a debug build `class Add: __add__ = lambda s, o: s + o; Add() + 1` segfaulted on the main thread; it now raises, as it does under CPython and in release builds. enter_iframe checked and then called enter_iframe_unchecked, which checked again; it now leaves the check to the one call. Measured on a call-dominated benchmark, five interleaved pairs: instructions retired go up 0.17%, about four per call, which is the stack pointer read and the compare. Assisted-by: Claude
`map_py_iter` read `__length_hint__` only to pass it to `PyIterIter`, and returned an empty vector when the hint was `isize::MAX` or more. Collecting through `PyResult` dropped the iterator's lower bound, so nothing reserved the room the hint asked for. `list()`, `list.extend()` and `list.__iadd__()` now reserve it and report a hint they cannot honour as `MemoryError`; a hint that leaves no room for the elements the list already holds is passed over, as `list_extend()` does. `tuple()` and the other callers keep filling up without reserving. `length_hint_opt` errors other than the `TypeError` it already turns into `None` now reach the caller instead of being dropped. `__iadd__` and `inplace_concat` went through `extract_cloned`, which reads `__len__` and not `__length_hint__`; both call `PyList::extend` now, the way `list_inplace_concat()` calls `list_extend()`. The tuple, list and dict fast paths of `extract_elements_inner` reserve their known length, which the `collect()` they used dropped. Assisted-by: Claude
`map_methods` has no `__length_hint__`, so `operator.length_hint()` on a map answers 0, not the length of what it draws from. The method walked into the length hint of every iterator it holds, and a chain of maps 10000 long overflowed the native stack answering for the outermost one. It also took the longest of its iterators, where a map stops at the shortest. Assisted-by: Claude
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
`map_py_iter` asked the iterable it was handed, for every caller, and reported what asking raised. Only some callers ask it: `list_extend()` and `_PyBytes_FromIterator()` ask the iterable, `PySequence_Tuple()` asks the iterator, and the bytearray constructor asks nothing. `tuple()`, `min()`, `max()`, `collections.deque()` and `f(*x)` raised for an iterable whose `__len__` or `__length_hint__` does, where they answer. Which object is asked is now the caller's to say. `sorted()` asks the iterable, being `PySequence_List()`. `bytes_from_object()` stood in for `PyBytes_FromObject()`, for `bytearray_extend()` and for the bytearray constructor, which do not agree on this: the first two ask, the last does not. It is split, and assigning to a bytearray slice takes the constructor's side with `PyByteArray_FromObject()`. `list.extend()` counted what it held before the iterable had been asked, where `list_extend()` reads `Py_SIZE(self)` after. A `__length_hint__` that adds to the list made the overflow guard read a count too small and raise `MemoryError` where nothing is wrong; one that empties it made the guard skip a reservation that cannot be served. Assisted-by: Claude
`product_new()` checks `repeat` and works out `npools` before it calls `PySequence_Tuple()` on any argument, and fills the pools `npools` times. The pools were filled by repeating the arguments `repeat` times instead, which walks that many steps even with no arguments to repeat: `product(repeat=2**62)` counted up to it rather than answering `[()]`. The count was also worked out after the arguments had been read, so a repeat too large to serve ran their code first. Assisted-by: Claude
`pack_single()` leaves `'?'` to `PyObject_IsTrue()` and returns what that raised. Packing classified the error instead, so a `ValueError` from a `__bool__` came back as "memoryview: invalid value for format '?'". Assisted-by: Claude
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nine changes from the current fuzzing and static-review sweep. Each one is a
separate commit with its own reasoning; the branch is rebased onto main, and
everything the last sweep already landed has been dropped from it.
Crashes that are now exceptions
struct.pack('%dx' % 2**60),(0).to_bytes(2**60, 'big'),os.read(fd, 2**60)and(ctypes.c_char * 2**60)()all reachedhandle_alloc_error, which underpanic = "abort"ends the process. They raiseMemoryErrornow, as they doin the reference.
itertools.product(..., repeat=2**60)and_ssl.RAND_bytes(2**40)raiseOverflowError;product_newgained thenegative and too-large checks it has upstream, and
RAND_bytestakes the Cintits signature says it does.check_c_stack_overflow()ran on every eighth frameentry, and the margin does not cover eight frames of a recursion that
re-enters through native code. A
class Add: __add__ = lambda self, o: self + ochain segfaulted on the main thread of a debug build, and a self-sorting
key=died onSIGILL. The check runs on every entry now; the cost is+0.17% instructions retired on a call-heavy benchmark (~4 instructions per
call).
map.__length_hint__.map_methodshas no__length_hint__, sooperator.length_hint()on a map answers 0. Ours walked into the length hintof every iterator it held, and a chain of maps 10000 long overflowed the
native stack answering for the outermost one. It also took the longest of its
iterators, where a map stops at the shortest.
threading.stack_size(256*1024)then starting athread aborted.
ExecutingFrame::runreserves 80,848 bytes in a debug buildagainst 656 in release —
execute_instructionis#[inline(always)]with 200instruction arms, and LLVM's StackColoring pass only runs at opt-level >= 1 —
so one Python call costs 88,672 bytes and the threading bootstrap's six frames
need 532,032. An explicit size is a floor rather than the size in debug builds
now; release honours it exactly and
threading.stack_size()still reportswhat was asked for.
Length hints
map_py_iterread__length_hint__only to hand it toPyIterIter, andreturned an empty vector outright when the hint was
isize::MAXor more — solist(x)wherex.__length_hint__()issys.maxsizeanswered[]. Collectingthrough
PyResultdrops the iterator's lower bound, so nothing reserved the roomthe hint asked for either.
list(),list.extend()andlist.__iadd__()reserve it now and report a hintthey cannot honour as
MemoryError; a hint that leaves no room for what the listalready holds is passed over, as
list_extend()does.tuple()keeps filling upwithout reserving, which is also what the reference does. Errors from
length_hint_optother than theTypeErrorit already turns intoNonereachthe caller.
__iadd__andinplace_concatwent throughextract_cloned, which reads__len__and not__length_hint__; both callPyList::extendnow, the waylist_inplace_concat()callslist_extend(). The tuple, list and dict fastpaths of
extract_elements_innerreserve their known length, which thecollect()they used dropped:list.extend()is 9.9% fewer instructions and alist-construction benchmark 1.9% fewer.
Who is asked for a length hint
map_py_iterasked the iterable it was handed, for every caller. Only somecallers ask it:
list_extend()and_PyBytes_FromIterator()ask the iterable,PySequence_Tuple()asks the iterator, and the bytearray constructor asksnothing. Which object is asked is the caller's to say now, so
tuple(),min(),max(),collections.deque()andf(*x)answer for an iterable whose__len__raises, where they used to report it;sorted()asks the iterable,being
PySequence_List().bytes_from_object()stood in forPyBytes_FromObject(), forbytearray_extend()and for the bytearray constructor, which do not agree onthis — the first two ask, the last does not. It is split, and assigning to a
bytearray slice takes the constructor's side with
PyByteArray_FromObject().list.extend()counted what it held before the iterable had been asked, wherelist_extend()readsPy_SIZE(self)after. A__length_hint__that adds tothe list made the guard read a count too small and raise
MemoryErrorwherenothing is wrong; one that empties it made the guard skip a reservation that
cannot be served.
Two more that turned up alongside, neither of them new:
product_new()works outnpoolsbefore it callsPySequence_Tuple()on anyargument, and fills the pools
npoolstimes. Ours filled them by repeatingthe arguments
repeattimes, which walks that many steps even with noarguments to repeat —
product(repeat=2**62)counted up to it rather thananswering
[()]— and worked the count out after reading the arguments, so arepeat too large to serve ran their code first.
pack_single()leaves'?'toPyObject_IsTrue()and returns what thatraised. Packing classified the error instead, so a
ValueErrorfrom a__bool__came back asmemoryview: invalid value for format '?'.Collector
The collection kept its candidates and their working counts in a side table
keyed by address. Both now live in the object header, in a
gc_refsfield thatfits in the padding a 64-bit header already had — the header is unchanged at 48
bytes there, and 28 rather than 24 bytes on 32-bit, which the wasm32 build
covers. On a GC benchmark the median live-object collection went from 0.101s to
0.055s and the dead-object one from 0.402s to 0.383s.
Smaller fixes
memoryviewitem assignment raisedTypeErrorfor a value of the right kindthat does not fit; it is a
ValueError, andstruct's checks and messagesare matched alongside it.
seek_fdon Windows checked only the low half of the file position, soINVALID_SET_FILE_POINTERcould not be told from a valid position with thesame low word.
Left alone
Two divergences found along the way want a restructure larger than this branch
should carry, and neither is a crash:
memoryview(...).cast('f')[0] = 3.5e38reportsValueErrorwherepack_single()casts and storesinf._structpacks the same formatthrough
PyFloat_Pack4(), which does raise, and onePackableimplementationserves both here.
os.read(fd, 2**60)reportsMemoryErrorwhere CPython returns the data:PyBytes_FromStringAndSize(NULL, n)leaves the buffer uninitialised, and apage-lazy
mallocserves what acallocwill not. Matching it needs afallible uninitialised buffer, which cannot be spelled soundly on stable Rust.
This branch already moves that call from aborting the process to raising.
Verification
after —
test.test_future_stmt.test_future(barry_as_FLUFLin the REPL,documented unimplemented) and
test_setreporting env changed fromcheck_free_after_iterating, both reproduced on an unmodified build.test_pyreplis excluded for taking 24 minutes to reach its existing 48failures.
clippyclean on both the main and the wasm command lines,rustfmtandruffclean, and the wasm32 build passes.