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

Have receivers enforce max-message-size if configured #57

Closed
herfort opened this issue Sep 22, 2017 · 6 comments
Closed

Have receivers enforce max-message-size if configured #57

herfort opened this issue Sep 22, 2017 · 6 comments
Milestone

Comments

@herfort
Copy link

herfort commented Sep 22, 2017

Hi,
I tried to use the property MaxMessageSize. On my set up the property is bigger on the sender side as on the receiver side and I send message that is bigger as both properties.
I would expect that the sender checks the own and the RemoteMaxMessageSize and the receiver throws error if the message is bigger than MaxMessageSize.
See attached file:
SetMaxMessageSizeTest.java.txt

@gemmellr
Copy link
Contributor

gemmellr commented Oct 5, 2017

The setter/accessors only convey/access information to/from the remote peer in the link attach, there is currently no enforcement by the library itself, which like proton-j (and -c) itself leaves it to the using application code to handle and react to.

In hindsight, given it is actually the fully encoded message data which that size value governs, and not of just e.g the string body used in this case, it was a mistake exposing the setters/getters in vertx-proton without also considering that since the encoded message isn't directly seen by the using application code there (assuming it doesn't encode it itself just to check it). Handling it internally on the receiving side the way the spec indicates would be a little awkward. Need to think on how to address that.

@sophokles73
Copy link
Contributor

sophokles73 commented Oct 9, 2017

I agree with your assessment, @gemmellr, that it is hard for the application layer to determine the total message size based on payload (and maybe headers). This should be left to the framework layer.
FMPOV this should be handled by both, sending as well as receiving, sides of the link. The sending side should refuse sending a message whose wire encoding is larger than the negotiated value, whereas the receiving side should refuse the reception of such messages and should be entitled to close the link with a corresponding error code (this should probably be configurable).

@sophokles73
Copy link
Contributor

@gemmellr

Handling it internally on the receiving side the way the spec indicates would be a little awkward. Need to think on how to address that.

My understanding of both max-frame-size as well as max-message-size is that these properties can be used to implement a safeguard on the receiver side to prevent a sender from flooding the receiver with frames that would eat up the receiver's memory. Based on that assumption I think that the receiver side will have to enforce both property values in order to provide this safeguard. If this is deferred to the application layer (on the receiver side) then problem will already have occurred way before the receiving application first sees the incoming message. If I am not mistaken then proton-j already enforces the max-frame-size on the receiving end of a link but it doesn't limit the inbound message buffer based on the max-message-size, does it?

@sophokles73
Copy link
Contributor

sophokles73 commented Sep 3, 2020

@gemmellr We are running into this problem again. Any thoughts regarding if and how to fix this at the vertx-proton and/or proton-j layer?
FMPOV this needs to be fixed at the proton-j layer as that is where the message is being assembled from the (multiple) transfer frames, right?

sophokles73 added a commit to bosch-io/vertx-proton that referenced this issue Sep 8, 2020
A receiver link now checks if the receiver side's max-message-size is
being exceeded by an incoming transfer frame's content. If so, the
transfer is being rejected, thus preventing senders from exceeding the
receiver's resource limits.

Signed-off-by: Kai Hudalla <kai.hudalla@bosch.io>
@sophokles73
Copy link
Contributor

@gemmellr After having taken a closer look at the vertx-proton code, it seems that multi-transfer messages are being assembled at the vertx-proton layer. I have created a PR which should prevent receivers from accepting transfers that would result in exceeding the max-message-size.

sophokles73 added a commit to eclipse-hono/hono that referenced this issue Sep 8, 2020
The AMQP adapter is now being tested to reject messages exceeding the
adapter's configured max-message-size. The test can be enabled with a
feature flag as the underlying vertx-proton library does not (yet)
support this. Thus, the test is disabled for the time being.

See vert-x3/vertx-proton#57

Signed-off-by: Kai Hudalla <kai.hudalla@bosch.io>
sophokles73 added a commit to bosch-io/vertx-proton that referenced this issue Sep 9, 2020
A receiver link now checks if the receiver side's max-message-size is
being exceeded by an incoming transfer frame's content. If so, the
application is notified via a handler and the link is being closed.

Signed-off-by: Kai Hudalla <kai.hudalla@bosch.io>
gemmellr added a commit that referenced this issue Sep 10, 2020
@gemmellr gemmellr added this to the 3.9.3 milestone Sep 10, 2020
gemmellr added a commit that referenced this issue Sep 10, 2020
…loses #57.

Tests largely based on those from by Kai Hudalla from PR #115.
gemmellr added a commit that referenced this issue Sep 10, 2020
…loses #57.

Tests largely based on those from by Kai Hudalla from PR #115.

(cherry picked from commit 6955463)
@gemmellr gemmellr changed the title using of "max message size" Have receivers enforce max-message-size if configured Sep 10, 2020
@gemmellr
Copy link
Contributor

Resolved via #116, pushed to master in 6955463 and backported to 3.9 in 4090c6f.

gemmellr added a commit that referenced this issue Sep 11, 2020
(cherry picked from commit e4f7cee with fixup)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants