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

fix: Use After Free in PacketReader #67

Merged
merged 8 commits into from
Sep 19, 2024
Merged

Conversation

discord9
Copy link
Contributor

@discord9 discord9 commented Sep 11, 2024

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

related issue: #66
Refactor packet_reader.rs to avoid Use After Free due to dropping Vec while still holding packets of u8 slice from it.

  • impl NomBytes type which allow nom to parse bytes::Bytes which is smart pointer to array of u8, hence avoid uaf
  • reuse buffer whenever possible to improve performance
  • make sure packet still have a lifetime arg, so to prevent breaking change to api
  • correct both impl of next() and next_async() to prevent UAF or unsafe code
  • add tests that run to those large packet dealing logics(Which disclosure another bug about having packet size of exactly U24_MAX would also cause nom to refuse parse packets due to lack of opt, fixed)
  • cargo test is passed

@discord9
Copy link
Contributor Author

@PsiACE might want to take a look?

@discord9
Copy link
Contributor Author

The style warning is all in clickhouse/*, I'm not sure if I should fix it in case might break something,
The MacOS CI failure seems to be related to wrong CI setting in Mac env?

@PsiACE
Copy link
Member

PsiACE commented Sep 18, 2024

Thanks @discord9 . I will check tonight, I am now a community contributor.

@PsiACE PsiACE merged commit 6bbc3b6 into databendlabs:main Sep 19, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants