Skip to content

common: fix UB in some bit shifts #1736

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

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

tlyu
Copy link
Contributor

@tlyu tlyu commented Jan 16, 2024

Fix a few instances of undefined behavior (that I'm not yet aware of any concrete consequences of) in left shift expressions. Roughly, it's undefined behavior to shift a 1 into the sign bit of a signed type.

Operands in bit shift expressions undergo integer promotions, but not the usual arithmetic conversions. Notably, the second operand can't promote the first operand to unsigned. Shifting 1 rather than 1U can lead to shifting a 1 into the sign bit, causing undefined behavior.

Relatedly, the integer promotions on a 32-bit platform will usually promote uint16_t to a signed int, leading to undefined behavior in left shift expressions.

Detailed description

  • No new feature
  • No known existing misbehavior or incorrect code generation, though it's theoretically possible
  • This fix increases standard conformance in bit shift expressions by using unsigned integer constants and explicitly casting unsigned narrow integers to wider unsigned integers.

Your checklist for this pull request

Closing issues

N/A

@tlyu
Copy link
Contributor Author

tlyu commented Jan 16, 2024

The binary did get about 8 bytes larger. I tried to diff the disassembly of the object files that got larger, and saw no changes to instructions, but only changes in the locations of some constant (debug?) strings.

@dragonmux dragonmux added this to the v2.0 release milestone Jan 16, 2024
@dragonmux dragonmux added Bug Confirmed bug Enhancement General project improvement BMP Firmware Black Magic Probe Firmware (not PC hosted software) labels Jan 16, 2024
@dragonmux
Copy link
Member

This does need the commit message fixing to include the proper prefix (either split it in two, or us prefix common:).

Otherwise, this looks good and we can trivially see it's correct so will merge when that's addressed.

Fix a few instances of undefined behavior (that I'm not yet aware of any
concrete consequences of) in left shift expressions. Roughly, it's
undefined behavior to shift a `1` into the sign bit of a signed type.

Operands in bit shift expressions undergo integer promotions, but not
the usual arithmetic conversions. Notably, the second operand can't
promote the first operand to unsigned. Shifting `1` rather than `1U` can
lead to shifting a `1` into the sign bit, causing undefined behavior.

Relatedly, the integer promotions on a 32-bit platform will usually
promote `uint16_t` to a signed `int`, leading to undefined behavior in
left shift expressions.
@tlyu tlyu changed the title fix UB in some bit shifts common: fix UB in some bit shifts Jan 17, 2024
@tlyu
Copy link
Contributor Author

tlyu commented Jan 17, 2024

Thanks! Updated. Also updated the PR title.

Copy link
Member

@dragonmux dragonmux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, good catches! Merging - thank you for the contribution.

@dragonmux dragonmux merged commit cea35a6 into blackmagic-debug:main Jan 17, 2024
@tlyu tlyu deleted the fix/lshift-ub branch January 17, 2024 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BMP Firmware Black Magic Probe Firmware (not PC hosted software) Bug Confirmed bug Enhancement General project improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants