Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MDEV-31566 Fix buffer overrun of column_json function #3242

Merged
merged 1 commit into from May 9, 2024

Conversation

tohghua
Copy link

@tohghua tohghua commented May 7, 2024

See https://jira.mariadb.org/browse/MDEV-31566

Test case: the following sql should work fine.
select column_json(0x0402000A0000000300030023076A736E7375626A6563742E0005006C0027000200290002002B0002002D0002002F0002000C31000C3B000C4B000C51000F62006631663266336634663509E5A79AE8BF9CE6B48B0FE8819AE9809AE98791E6A1A5E5BA970537343530301031313634332F393634352F31313630300C080000000000EFBFBDEFBFBD192E);
select column_json(0x0402000900000003000300740C6A736E766F6C756D652E000900EFBFBD004300020045000200470003004A0004004E00050053000500580005005D000500620005000C67000C6A000C6D000C7000052C00051B00052C000CEFBFBD0007EFBFBD006638663966313070696332626F785F63626F785F67626F785F6B626F785F7666355F696402343402343402333241687474703A2F2F6F73732E68646238382E636F6D2F302F70686F746F2F30373865653765376336343634616236386130343833373333323636613532612E67696608302E303532323732244F1E00030180C106);

@tohghua tohghua changed the title Fix buffer overrun in dynstr_append_json_quoted() MDEV-31566 Fix buffer overrun in dynstr_append_json_quoted() May 7, 2024
@tohghua tohghua changed the title MDEV-31566 Fix buffer overrun in dynstr_append_json_quoted() MDEV-31566 Fix buffer overrun of column_json function May 7, 2024
@anson1014
Copy link
Contributor

Hi @tohghua, please follow the PR template for contributions

<!--
Thank you for contributing to the MariaDB Server repository!

You can help us review your changes faster by filling in this template <3

If you have any questions related to MariaDB or you just want to hang out and meet other community members, please join us on https://mariadb.zulipchat.com/ .
-->

<!--
If you've already identified a https://jira.mariadb.org/ issue that seems to track this bug/feature, please add its number below.
-->
- [x] *The Jira issue number for this PR is: MDEV-______*

<!--
An amazing description should answer some questions like:
1. What problem is the patch trying to solve?
2. If some output changed that is not visible in a test case, what was it looking like before the change and how it's looking with this patch applied?
3. Do you think this patch might introduce side-effects in other parts of the server?
-->
## Description
TODO: fill description here

## Release Notes
TODO: What should the release notes say about this change?
Include any changed system variables, status variables or behaviour. Optionally list any https://mariadb.com/kb/ pages that need changing.

## How can this PR be tested?

TODO: modify the automated test suite to verify that the PR causes MariaDB to behave as intended.
Consult the documentation on ["Writing good test cases"](https://mariadb.org/get-involved/getting-started-for-developers/writing-good-test-cases-mariadb-server).
<!--
In many cases, this will be as simple as modifying one `.test` and one `.result` file in the `mysql-test/` subdirectory.
Without automated tests, future regressions in the expected behavior can't be automatically detected and verified.
-->

If the changes are not amenable to automated testing, please explain why not and carefully describe how to test manually.

<!--
Tick one of the following boxes [x] to help us understand if the base branch for the PR is correct.
see [CODING_STANDARDS.md](https://github.com/MariaDB/server/blob/-/CODING_STANDARDS.md) for the latest versions.
-->
## Basing the PR against the correct MariaDB version
- [ ] *This is a new feature and the PR is based against the latest MariaDB development branch.*
- [ ] *This is a bug fix and the PR is based against the earliest maintained branch in which the bug can be reproduced.*

<!--
  All code merged into the MariaDB codebase must meet a quality standard and codying style.
  Maintainers are happy to point out inconsistencies but in order to speed up the review and merge process we ask you to check the CODING standards.
-->
## PR quality check
- [ ] I checked the [CODING_STANDARDS.md](https://github.com/MariaDB/server/blob/-/CODING_STANDARDS.md) file and my PR conforms to this where appropriate.
- [ ] For any trivial modifications to the PR, I am ok with the reviewer making the changes themselves.

Furthermore, your commit message is very vague and does not describe what the issue you are fixing is, why you are fixing it or when it occurs. Please consider adding some more detail. Furthermore, you describe in the current PR description a test case that should pass with the addition of your new changes. Perhaps add an MTR test to verify this as well.

Here is some good documentation you can refer to for what I've mentioned above:

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

Implementation looks good.

Thanks for signing the CLA.

The commit message will need to be improved and test case added for this to merge (back to 10.5 branch).

If you want use to take care of these details please let us know.

mysys/ma_dyncol.c Show resolved Hide resolved
@@ -3865,17 +3865,18 @@ my_bool dynstr_append_json_quoted(DYNAMIC_STRING *str,
}
else
{
if (lim < 2)
Copy link
Member

Choose a reason for hiding this comment

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

so we moved this because there could be up to 2 more characters. We could be over allocating, but I'm not sure it matters.

Copy link
Author

@tohghua tohghua May 8, 2024

Choose a reason for hiding this comment

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

Over allocating should be fine, it's just a buffer.

If you want us to take care of these details please let us know.
You're welcome :)

lim--;
str->str[str->length++]= '\\';
}
lim--;
Copy link
Member

Choose a reason for hiding this comment

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

good.

The accounting of the limit variable that represents the
amount of space left it the buffer was incorrect.

Also there was 1 or 2 bytes left to write that occured without
the buffer length being checked.

Review: Sanja Byelkin
@grooverdan grooverdan changed the base branch from 11.5 to 10.5 May 9, 2024 00:05
@CLAassistant
Copy link

CLAassistant commented May 9, 2024

CLA assistant check
All committers have signed the CLA.

@grooverdan grooverdan enabled auto-merge (rebase) May 9, 2024 00:05
@grooverdan
Copy link
Member

thank you for the fix @tohghua. Sorry I was slow applying your first fix.

@grooverdan grooverdan merged commit 8677472 into MariaDB:10.5 May 9, 2024
13 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants