check row buffer bounds in Field_new_decimal::unpack#5175
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 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
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.
2 participants
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.