Skip to content

check row buffer bounds in Field_new_decimal::unpack#5175

Open
jmestwa-coder wants to merge 1 commit into
MariaDB:10.11from
jmestwa-coder:fix-decimal-unpack-oob
Open

check row buffer bounds in Field_new_decimal::unpack#5175
jmestwa-coder wants to merge 1 commit into
MariaDB:10.11from
jmestwa-coder:fix-decimal-unpack-oob

Conversation

@jmestwa-coder
Copy link
Copy Markdown
Contributor

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.

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 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.

Copy link
Copy Markdown
Member

@grooverdan grooverdan left a comment

Choose a reason for hiding this comment

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

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.

Comment thread sql/field.cc Outdated
a decimal and write that to the raw data buffer.
*/
if (from + from_pack_len > from_end)
return 0; // Wrong data
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if both branches of the encompasing if statement do this condition, I think it should be executed earlier.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@gkodinov gkodinov added the External Contribution All PRs from entities outside of MariaDB Foundation, Corporation, Codership agreements. label Jun 4, 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.

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.
@jmestwa-coder jmestwa-coder force-pushed the fix-decimal-unpack-oob branch from b95af30 to 857f62b Compare June 4, 2026 12:38
@jmestwa-coder jmestwa-coder changed the base branch from main to 10.11 June 4, 2026 12:38
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jun 4, 2026

CLA assistant check
All committers have signed the CLA.

@jmestwa-coder
Copy link
Copy Markdown
Contributor Author

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.

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.

4 participants