Skip to content

bound shift width in dyncol integer readers#5154

Open
jmestwa-coder wants to merge 1 commit into
MariaDB:mainfrom
jmestwa-coder:dyncol-int-shift-width
Open

bound shift width in dyncol integer readers#5154
jmestwa-coder wants to merge 1 commit into
MariaDB:mainfrom
jmestwa-coder:dyncol-int-shift-width

Conversation

@jmestwa-coder
Copy link
Copy Markdown
Contributor

Noticed the dyncol integer readers shift by an exponent taken from the value length. In dynamic_column_uint_read() the i*8 shift reaches 64 once a COLUMN_GET() blob gives an integer column more than 8 data bytes, and dynamic_column_var_uint_get() keeps shifting length*7 over a run of 0x80 continuation bytes (the charset id of a string value, intg/frac of a decimal). Both are undefined and abort under -fsanitize=shift. Reject the over-long integer and cap the varint at 10 groups.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces safety checks in mysys/ma_dyncol.c to prevent undefined behavior from bitwise shifts exceeding 64 bits. Specifically, it limits the loop in dynamic_column_var_uint_get to 10 iterations, rejects lengths greater than 8 in dynamic_column_uint_read, and ensures that dynamic_column_sint_read correctly propagates errors returned by dynamic_column_uint_read. No review comments were provided, and the changes appear correct.

@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Jun 2, 2026
Copy link
Copy Markdown
Member

@gkodinov gkodinov left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. This is a preliminary review!

This needs:

  • a MDEV describing the problem, complete with steps to reproduce
  • a commit message compliant with the MariaDB coding standards
  • a regression test
  • a rebase to the lowest affected version (this is a bug fix and not a feature).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements.

Development

Successfully merging this pull request may close these issues.

2 participants