Skip to content

STOMP: introduce a max frame size limit #8802

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

Merged
merged 9 commits into from
Jul 8, 2023

Conversation

ikavgo
Copy link
Contributor

@ikavgo ikavgo commented Jul 7, 2023

Proposed Changes

Add frame size limit.

The default is 4 MiB.

Cloeses #2567.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • I have added tests that prove my fix is effective or that my feature works
  • All tests pass locally with my changes
  • If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
  • If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Further Comments

I would like to have it inside parser, it's way more intrusive in parser. It's a work in progress, but this size limit needed urgently so here it is. I will migrate it to parser via bigger PR.

@ikavgo ikavgo force-pushed the ik-stomp-frame-size-limit branch from ef34f33 to 75cec6f Compare July 8, 2023 11:11
@ikavgo ikavgo requested a review from michaelklishin July 8, 2023 11:27
@ikavgo
Copy link
Contributor Author

ikavgo commented Jul 8, 2023

@michaelklishin @mkuratczyk I'm open to changing the default limit. Also current implementation is coarse with tolerance up to buffer size (well it read from network already anyway). If end of the frame is inside of newly read buffer and it exceeds the limit parser is allowed to continue and return frame. in the next round I will move this check inside parser itself.

@michaelklishin michaelklishin changed the title Add configurable stomp frame size limit, default to 192kb. STOMP: make frame size limit configurable, default to 192 kiB Jul 8, 2023
deadtrickster and others added 2 commits July 8, 2023 20:44
(cherry picked from commit 5978c15f4ceac0b1965bcce5017766a23987a2af)
@michaelklishin michaelklishin added this to the 3.12.2 milestone Jul 8, 2023
@michaelklishin michaelklishin changed the title STOMP: make frame size limit configurable, default to 192 kiB STOMP: make frame size limit configurable Jul 8, 2023
@michaelklishin michaelklishin modified the milestones: 3.12.2, 3.13.0 Jul 8, 2023
@michaelklishin
Copy link
Collaborator

Let's start with a 1 MiB limit and ship this in 3.13.0.

@ikavgo
Copy link
Contributor Author

ikavgo commented Jul 8, 2023

why no backport to 3.12?

@michaelklishin
Copy link
Collaborator

Because previously there was no limit and now there is, and any system where STOMP frame size exceeds it will run into rejected publishes.

@ikavgo
Copy link
Contributor Author

ikavgo commented Jul 8, 2023

I see it more like a critical bug fix. But I'll gladly accept your POV.

@michaelklishin
Copy link
Collaborator

michaelklishin commented Jul 8, 2023

Maybe a higher limit such as 4 MiB would be acceptable. I'm OK with using that and backporting. Let me bump the limit one more time.

@michaelklishin michaelklishin changed the title STOMP: make frame size limit configurable STOMP: introduce a max frame size limit Jul 8, 2023
@michaelklishin michaelklishin modified the milestones: 3.13.0, 3.12.2 Jul 8, 2023
@michaelklishin
Copy link
Collaborator

The OCI build failures seem unrelated.

@michaelklishin michaelklishin merged commit c4e459e into main Jul 8, 2023
@michaelklishin michaelklishin deleted the ik-stomp-frame-size-limit branch July 8, 2023 21:18
michaelklishin added a commit that referenced this pull request Jul 9, 2023
STOMP: introduce a max frame size limit (backport #8802)
@michaelklishin michaelklishin removed this from the 3.12.2 milestone Jul 12, 2023
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.

3 participants