gh-148286: Fix undefined behaviour in the ctypes bit field accessors - #154912
Open
matthiasgoergens wants to merge 1 commit into
Open
gh-148286: Fix undefined behaviour in the ctypes bit field accessors#154912matthiasgoergens wants to merge 1 commit into
matthiasgoergens wants to merge 1 commit into
Conversation
…ssors BIT_MASK, GET_BITFIELD and SET in Modules/_ctypes/cfield.c did their bit manipulation in the bit field's own type. When that type is signed, both shifting a negative value left and shifting a one into the sign bit are undefined behaviour, so UBSan reported, among others: cfield.c:644: left shift of 1 by 63 places cannot be represented in 'int64_t' cfield.c:640: signed integer overflow: -2147483648 - 1 cfield.c:636: left shift of negative value -32768 Do the arithmetic in uint64_t -- the widest type the accessors support -- and convert back to the field's own type at the end. BIT_MASK is replaced by LOW_BITS_MASK(), which no longer needs a type argument now that the mask is always computed unsigned. GET_BITFIELD takes the field type so it can still sign-extend signed fields, and is wrapped in do/while(0). Verified against the old macros with an exhaustive differential test: for every fixed-width type (int8_t..uint64_t), every num_bits and low_bit combination, ten storage patterns and twenty stored values -- 1,167,600 cases, no behaviour change, under both gcc and clang. The two Modules/_ctypes/cfield.c entries in Tools/ubsan/suppressions.txt are no longer needed. With them removed, test_ctypes passes under UBSAN_OPTIONS=halt_on_error=1; before this change it aborts at cfield.c:644 in i64_set.
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.
BIT_MASK,GET_BITFIELDandSETinModules/_ctypes/cfield.cdo their bit manipulation in the bit field's own type. When that type is signed, both shifting a negative value left and shifting a one into the sign bit are undefined behaviour, which is whyTools/ubsan/suppressions.txtcarries two file-level entries for this file.Removing those entries and running
test_ctypesunderUBSAN_OPTIONS=halt_on_error=1aborts immediately atcfield.c:644ini64_set. Collecting rather than halting shows diagnostics at three distinct lines, so the suppression was covering rather more than its comment documents:This does the arithmetic in
uint64_t— the widest type the accessors support — and converts back to the field's own type at the end.BIT_MASKbecomesLOW_BITS_MASK(), which no longer needs a type argument now that the mask is always computed unsigned;GET_BITFIELDgains the field type so it can still sign-extend signed fields, and is wrapped indo { } while (0).Since this rewrites the arithmetic of every integer bit field getter and setter, I checked it exhaustively against the old macros rather than relying on review alone. For every fixed-width type from
int8_ttouint64_t, everynum_bits, everylow_bit, ten storage bit patterns and twenty stored values — 1,167,600 cases — the old and new expressions produce identical results, under both gcc and clang. The same sweep under-fsanitize=undefined -fno-sanitize-recover=allis clean, where the old macros produce eleven distinct diagnostics.With the two
Modules/_ctypes/cfield.centries removed from the suppressions file,test_ctypespasses underUBSAN_OPTIONS=halt_on_error=1.