check row buffer bounds in Field_new_decimal::unpack#5175
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a bounds check in the Field_new_decimal::unpack function in sql/field.cc to ensure that the data being unpacked does not exceed the end of the buffer (from_end). If the packed length exceeds the buffer limit, the function now returns 0 to indicate invalid data, preventing potential out-of-bounds reads. There are no review comments, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
grooverdan
left a comment
There was a problem hiding this comment.
Looks like a good find. Thanks and well described.
As this is a bug fix it should target 10.11 (rebase, git force push, and edit PR title to change target branch).
A test case would be most appreciated - mysql-test/suite/rpl/t/rpl_colSize.test seems like a good location for the change. mtr --record rpl.rpl_colSite and commit the generated .result file. Note current tests have the form:
# END 5.1 Test Case
...
--echo #
--echo # MDEV-xxxx check row buffer bounds in Field_new_decimal::unpack
--echo #
(test)
---echo # End of 10.11 test
--source include/rpl_end.inc
Can you create an issue on https://jira.mariadb.org/browse/.
Then use that MDEV in the commit message and PR title.
Your PR has a good description, bug can you put the problem description and solution in the commit message as well.
Much appreciate the contribution. I hope this extra effort for the benefit of the longer term maintenance of MariaDB codebase is manageable. Do ask if you have questions. If you need guidance you can ask on https://mariadb.zulipchat.com or on this PR.
| a decimal and write that to the raw data buffer. | ||
| */ | ||
| if (from + from_pack_len > from_end) | ||
| return 0; // Wrong data |
There was a problem hiding this comment.
if both branches of the encompasing if statement do this condition, I think it should be executed earlier.
There was a problem hiding this comment.
moved it above the if. from_pack_len is the size the conversion branch reads and it's always >= len, so the single check covers the copy branch too.
There was a problem hiding this comment.
Thank you for your contribution. This is a preliminary review.
Please do the following:
- Try to provide a test case. You already have some idea on what the scenario is. Please consider adding a check.
- Please back-port to the lowest affected version. I see that 10.11 exposes the same code for this method.
- Consider moving the check from the else branch to the top of the if(). No need to maintain 2 separate checks.
- Please file a jira explaining the problem and the solution.
Field_new_decimal::unpack takes the conversion branch when the master's decimal precision or scale differ from the slave's, then calls bin2decimal, which reads my_decimal_get_binary_size(from_precision, from_decimal) bytes from the row buffer. unlike the copy branch right below it, and unlike the other Field::unpack overrides, it never checked that length against from_end, so a truncated row-based binlog event made the read run past row_end (heap out-of-bounds read). move the from_end check above the if so a single test covers both branches. from_pack_len is the number of bytes the conversion branch reads and is always >= len, the size the copy branch reads, so one check up front is sufficient.
b95af30 to
857f62b
Compare
|
pushed an update. moved the bounds check above the if so both branches share one test (from_pack_len is what the conversion branch reads and it's always >= len, so it covers the copy branch too). rebased onto 10.11 and retargeted the PR, and moved the problem/solution writeup into the commit message. still to do on my side: I'll file an MDEV and fold it into the title and commit, and add a case to rpl_colSize.test, recording the .result with mtr --record. |
Field_new_decimal::unpack takes the conversion branch when the master's decimal precision or scale differ from the slave's, then calls bin2decimal, which reads my_decimal_get_binary_size(from_precision, from_decimal) bytes from the row buffer. unlike the copy branch right below it (and the other Field::unpack overrides) it never checks that length against from_end, so a truncated row-based binlog event makes the read run past row_end. adds the from_end check the copy branch already does.