bound shift width in dyncol integer readers#5154
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
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).
The dynamic column integer decoders derive the shift exponent from the data interval length without bounding it to the 64-bit type width. dynamic_column_uint_read() loops over the interval doing value+= data[i] << (i*8). A record whose integer column has an interval longer than 8 bytes drives i*8 to 64 and past it, which is undefined and aborts under -fsanitize=shift. dynamic_column_var_uint_get(), used for the charset id of a string value and for the intg/frac of a decimal, has the same defect: length*7 grows without bound over a run of 0x80 continuation bytes. Reject integers longer than 8 bytes with ER_DYNCOL_FORMAT, propagate that through dynamic_column_sint_read(), and cap the varint loop at 10 groups. To reproduce, build with -fsanitize=shift and read with COLUMN_GET() a record whose integer column data interval exceeds 8 bytes; the shift exponent reaches 64 in dynamic_column_uint_read(). A new case in the ma_dyncol unit test crafts such a record and checks it is rejected.
jmestwa-coder
force-pushed
the
dyncol-int-shift-width
branch
from
6ed15ce to
c3d1198
Compare
|
Reworked the commit message and rebased the branch onto 10.6, so the PR now targets the lowest affected version. Same fix to the three readers, just replanted. Added a regression case to the ma_dyncol unit test (test_overlong_int): it crafts a record whose integer column has a data interval longer than 8 bytes and checks that get rejects it as ER_DYNCOL_FORMAT. Without the bound the i*8 shift reaches 64 and aborts under -fsanitize=shift. The repro is written up in the commit message. I'll file an MDEV for it and add the number to the subject once it's assigned, unless you'd rather create one on your side. |
2 participants
Noticed the dyncol integer readers shift by an exponent taken from the value length. In dynamic_column_uint_read() the
i*8shift reaches 64 once a COLUMN_GET() blob gives an integer column more than 8 data bytes, and dynamic_column_var_uint_get() keeps shiftinglength*7over 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.