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

expression: handle corrupted length in uncompress builtin function #8586

Merged
merged 7 commits into from
Dec 6, 2018

Conversation

ziyi-yan
Copy link
Contributor

@ziyi-yan ziyi-yan commented Dec 5, 2018

What problem does this PR solve?

fixes #8578

What is changed and how it works?

Returns nil with warning when the length in first four bytes is less than the length of the returned string.

While fixing this issue, I change the error class of errZlibZData from ClassTypes to ClassExpression in order to display their SQL error codes correctly.

Check List

Tests

  • Unit test
  • Integration test

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

This change is Reviewable

@sre-bot
Copy link
Contributor

sre-bot commented Dec 5, 2018

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@ziyi-yan ziyi-yan force-pushed the fix-uncompress-corrupted-length branch from 36702cd to d43bb87 Compare December 5, 2018 18:05
@ziyi-yan
Copy link
Contributor Author

ziyi-yan commented Dec 5, 2018

PTAL @zimulala

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala
Copy link
Contributor

zimulala commented Dec 6, 2018

/run-all-tests

@shenli shenli added the contribution This PR is from a community contributor. label Dec 6, 2018
@ziyi-yan
Copy link
Contributor Author

ziyi-yan commented Dec 6, 2018

ping @zimulala @shenli

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason
Copy link
Member

zz-jason commented Dec 6, 2018

/run-all-tests

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. and removed type/compatibility labels Dec 6, 2018
@ziyi-yan
Copy link
Contributor Author

ziyi-yan commented Dec 6, 2018

Are there any problems blocking this PR?

@zz-jason zz-jason merged commit e61bad0 into pingcap:master Dec 6, 2018
@zz-jason
Copy link
Member

zz-jason commented Dec 6, 2018

@ziyi-yan Could you help us cherry-pick this commit to release-2.1 and release-2.0?

@ziyi-yan
Copy link
Contributor Author

ziyi-yan commented Dec 6, 2018

@zz-jason I would love to do it. But what am I supposed to do exactly? (I am not an experienced contributor yet.)
I might run git cherry-pick <this-commit-id> to branch release-2.0 and release-2.1 and make two PRs to these branches. Is that correct?

@zz-jason
Copy link
Member

zz-jason commented Dec 6, 2018

@zz-jason I would love to do it. But what am I supposed to do exactly? (I am not an experienced contributor yet.)
I might run git cherry-pick <this-commit-id> to branch release-2.0 and release-2.1 and make two PRs to these branches. Is that correct?

Yes, that's right. Feel free to ask if you encountered any problem. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builtin uncompress function ignored the four bytes length
6 participants