Skip to content

Fix undefined behaviour in GET macros due to signed shift - #1035

Merged
NWilson merged 1 commit into
mainfrom
user/niwilson/ub-get-shift
Oct 10, 2026
Merged

NWilson merged 1 commit into
mainfrom
user/niwilson/ub-get-shift

Conversation

@NWilson

@NWilson NWilson commented Oct 8, 2026

Copy link
Copy Markdown
Member

There is a comment about an odd integer promotion. This is because unsigned char promotes to signed int, legitimately as there is no truncation, but the shift should be done on an unsigned value to avoid shifting bits into the sign.

There is a comment about an odd integer promotion. This is because
unsigned char promotes to signed int, legitimately as there is no
truncation, but the shift should be done on an unsigned value to avoid
shifting bits into the sign.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The casts correctly eliminate signed-shift undefined behavior, and the sanitizer test counts and configurations remain consistent.

0 open findings

What changed in this PR

Fixes undefined behavior in internal offset-reading macros by ensuring shifts operate on unsigned values.

Changes:

  • Casts code units before shifts in GET and GET2.
  • Expands sanitizer testing across minimum and maximum link sizes.
  • Clarifies macro and test documentation.
File Description
src/​pcre2_intmodedep.h Makes offset reads and writes explicitly unsigned and width-safe.
maint/​ManyConfigTests Runs ASan and UBSan tests with link sizes 2 and 4.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@NWilson
NWilson merged commit ead0e18 into main Oct 10, 2026
45 checks passed
@NWilson
NWilson deleted the user/niwilson/ub-get-shift branch October 10, 2026 09:03
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