Repository navigation
avoid signed left-shift overflow in binary long/float/double readers - #4044
Open
ArjunD3456 wants to merge 1 commit into
Open
ArjunD3456 wants to merge 1 commit into
ArjunD3456 wants to merge 1 commit into
Conversation
read_long shifts each varint byte into a signed int64_t, and the big-endian branches of read_float and read_double shift a byte into the sign bit of a signed int32_t/int64_t. A full-width value shifts into bit 31/63, which is signed left-shift overflow and undefined. Accumulate in unsigned and assign to the signed destination, matching how the C++ BinaryDecoder already decodes varints.
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.
What is the purpose of the change
read_longinlang/c/src/encoding_binary.cbuilds a varint withvalue |= (int64_t)(b & 0x7F) << (7 * offset). On a 10-byte varint the last byte is shifted by 63 into a signedint64_t, so a payload bit lands on the sign bit and the shift overflows, which is undefined behavior. The big-endian branches ofread_floatandread_doublehave the same shape, shifting a byte into bit 31 of a signedint32_t(read_float) and bit 63 of a signedint64_t(read_double). All three feed on bytes taken straight from the input stream, so any Avro datum carrying a full-widthlong, or afloat/doubleread on a big-endian build, trips it.-fsanitize=undefinedreportsleft shift of ... cannot be represented in type 'long'onread_longwhile decodingINT64_MIN/INT64_MAX. The C++BinaryDecoder::doDecodeLongalready accumulates into an unsigneduint64_tfor this reason, so this brings the C reader in line: shift in unsigned and assign to the signed destination. Decoded values are unchanged on the platforms Avro targets.Verifying this change
This change is already covered by existing tests: the
long,float, anddoubleround-trip cases inlang/c/tests/test_avro_values.cexercise these readers (thelongcase decodes maximal 10-byte varints), and a-fsanitize=undefinedbuild of that suite is clean after the change where it previously reported the shift overflow. Behavior is unchanged for valid input.Documentation