Skip to content

fix undefined left shift of negative int in math.bitShiftLeft#2044

Merged
copybara-service[bot] merged 1 commit into
google:masterfrom
sahvx655-wq:bitshiftleft-signed-ub
Jun 5, 2026
Merged

fix undefined left shift of negative int in math.bitShiftLeft#2044
copybara-service[bot] merged 1 commit into
google:masterfrom
sahvx655-wq:bitshiftleft-signed-ub

Conversation

@sahvx655-wq
Copy link
Copy Markdown
Contributor

Noticed bitShiftLeft shifts a signed int64 directly while bitShiftRight goes through an unsigned bit_cast first. For a negative left operand like math.bitShiftLeft(-1, 1) that signed left shift is undefined behaviour, which ubsan flags as left shift of negative value. Shifting in the unsigned domain the way the right shift already does keeps the same bit pattern result without the UB. Added a couple of negative-operand cases to the runtime tests since they weren't covered before.

@jnthntatum
Copy link
Copy Markdown
Collaborator

/gcbrun

@sahvx655-wq
Copy link
Copy Markdown
Contributor Author

Thanks for kicking off the build. Quick recap of what it's exercising: BitShiftLeftInt was doing the shift straight on the signed int64_t, so any negative left operand like math.bitShiftLeft(-1, 1) tripped ubsan with "left shift of negative value". bitShiftRight already routed through an unsigned bit_cast, so this just brings the left shift into line and preserves the two's-complement bit pattern without the UB.

The two negative-operand cases I added to math_ext_test.cc (-1 << 1 and -4 << 2) cover the path that was previously undefined and wasn't exercised before. Worth noting the function is reachable directly from an untrusted expression, so on a ubsan or hardened build the current behaviour is an abort rather than a defined result. The change is contained to that one function, so nothing else in the file is affected.

@copybara-service copybara-service Bot merged commit a75e273 into google:master Jun 5, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants