dont check frame max size when parse

[ohroy]

When you first saw this you must have thought I was crazy and wanted to close this pr immediately, but I would like to ask you to bear with me and listen to my reasons!

I understand that the AMQP 0-9-1 protocol requires that the server and client must respect frame max

frame-max: the largest frame size that the server proposes for the connection. The client can negotiate a lower value. Zero means that the server does not impose any specific limit but may reject very large frames for internal reasons.

Once negotiated, both parties MUST NOT send frames exceeding this size.

When a message is too large to fit within a single frame, it must be split into multiple frames.

However, in reality, some cloud server vendors do not necessarily comply with this specification for cost reasons, such as alibabacloud https://www.alibabacloud.com/en/product/rabbitmq?plc=1, whatever you set frame max, the server never spilt frame, even they said frame size 0x8000 when tune.

You may be in disbelief, why would they dare to do this? But in fact, most AMQP libraries are tolerant of this, especially the official Java library of RabbitMQ.

    private void consumeHeaderFrame(Frame f) throws IOException {
        if (f.getType() == AMQP.FRAME_HEADER) {
            this.contentHeader = AMQImpl.readContentHeaderFrom(f.getInputStream());
            long bodySize = this.contentHeader.getBodySize();
            if (bodySize >= this.maxBodyLength) {
                throw new IllegalStateException(format(
                    "Message body is too large (%d), maximum configured size is %d. " +
                        "See ConnectionFactory#setMaxInboundMessageBodySize " +
                        "if you need to increase the limit.",
                    bodySize, this.maxBodyLength
                ));
            }
            this.remainingBodyBytes = bodySize;
            updateContentBodyState();
        } else {
            throw new UnexpectedFrameError(f, AMQP.FRAME_HEADER);
        }
    }

https://github.com/rabbitmq/rabbitmq-java-client/blob/d5b8b673768379e42056a27f5dd8c0e9bda64812/src/main/java/com/rabbitmq/client/impl/CommandAssembler.java#L104-L108

Another nodejs library does the same.

export async function decodeFrame(read: (bytes: number) => Promise<Buffer>): Promise<DataFrame> {
  const chunk = await read(7)
  const frameTypeId = chunk.readUint8(0)
  const channelId = chunk.readUint16BE(1)
  const frameSize = chunk.readUint32BE(3)
  const payloadBuffer = await read(frameSize + 1)
  if (payloadBuffer[frameSize] !== FRAME_END)
    throw new AMQPConnectionError('FRAME_ERROR', 'invalid FRAME_END octet: ' + payloadBuffer[frameSize])
  if (frameTypeId === FrameType.METHOD) {

https://github.com/cody-greene/node-rabbitmq-client/blob/c44a4b48c03d5a43f9d98e1979246bebd62143bc/src/codec.ts#L696C1-L704C42

[ohroy]

As a digression, I spent days tracking down this issue

From oven-sh/bun#5627, for the test repo https://github.com/acuminous/amqplib-764, I follow the comment patched stream to readable-stream, but it not work. and then I drop bun replace by node, it still gets the same error.

Finally, I found the reason is rabbit from alibaba cloud, when use local docker version rabbitmq, it works even with bun. (So I think bun had fixed that bug...)

[cressie176]

Hi @ohroy

Thank you for the PR and clear explanation. I did indeed think it was crazy until I read it!

I would prefer to try some alternatives...

• What happens if you set the frameMax to 0 indicating no limit?
• Rather than remove the check, could it be conditionally ignored, by way of a new client only tuning parameter, repurposing the existing frameMax parameter (not sure I like this), or failing this an environment variable

[ohroy]

What happens if you set the frameMax to 0 indicating no limit?

Setting framemax to 0 in this example doesn't work, because the server still returns a framemax when negotiating, but it doesn't respect it, so no matter how big it is, it doesn't solve the problem. (In fact, I've tried setting it to 0, or setting it to a very large value of '0xffffffff').
On the other hand, setting it to 0 will cause the client to stop splitting the frame.

Rather than remove the check, could it be conditionally ignored, by way of a new client only tuning parameter, repurposing the existing frameMax parameter (not sure I like this), or failing this an environment variable

This is, of course, a safer approach, and I've thought about it before. But after consulting many official libraries, I changed my mind for two main reasons:

1. In the official library, there is no check in this regard. I've consulted the official Java and Go libraries, and there are no such restrictions. https://github.com/rabbitmq/amqp091-go/blob/566601c8e5debdfbc715aa1ef9ca8220beee997a/read.go#L46
   If we increase this limit, it will be a huge confusion for people who don't know the protocol, and even blame us for bugs.
2. In most cases, the end of the verification is 0xCE enough to confirm that the packet is correct. This judgment is not necessary in 99% of cases, unless there is a bug on the server, which should be very rare. Checking every time because of this polar probability is a bit less worth it in terms of performance. In addition, if such a problem occurs, no matter whether it is checked or not, an exception will definitely occur, and there will be no change in the results. (because it's also impossible for the user to recover from frameError)

Overall, I can very much understand your feelings about giving in to the wrong person when it is clear that you are the right one.

This is not an urgent issue, as for me I can fix the problem with a patch. But for others who don't know the truth, this issue will make them understand who is right.

[cressie176]

Hi @ohroy, thank you for providing your rationale - it is very well considered. I've looked through the Java Client and agree - while frameMax is used to split frames on transmit, I can only see maxInboundMessageBodySize being used when parsing frames. As such I am inclined to approve.

@michaelklishin, @carlhoerberg - you both understand the amqp protocol better than me, do you have any thoughts on the matter?

[michaelklishin]

@cressie176 several other clients do something similar but not because they target a particular RabbitMQ-as-a-Service providers, it just happens to be a reasonably natural way of reading a complete frame.

I don't have an opinion on this PR.

On one hand, most public cloud providers contribute effectively or literally nothing to RabbitMQ, so they can go hug a tree and develop their own client libraries instead of getting everything for free from the RabbitMQ core team and various contributors.

On the other hand, as long as a full frame can be consumed this way, why not.

Across the projects the core team maintains, any other issue specific to Alibaba Cloud (or any other RabbitMQ-as-a-Service provider that contributes a typo fix every 12 months), would be closed. Cloud providers who do not contribute meaningfully and regularly can go hug a tree.

But this PR is functional and not unreasonable, and seemingly comes from a user who just wants to scratch their own itch, so why not.

[cressie176]

Thanks @michaelklishin

[michaelklishin]

the server still returns a framemax when negotiating, but it doesn't respect it, so no matter > how big it is, it doesn't solve the problem

FTR, this is factually incorrect. RabbitMQ does respect the negotiated frame_max. frame_max is the limit beyond which a frame — and this usually means a basic.publish body frame since
all other frames are always much smaller than the negotiated value — will be split into
multiple frames.

It does not mean that every frame a client receives will be frame_max in size. A small frame such as queue.delete or even a basic.publish frame sequence with small messages
and mostly default metadata will be way below the default and all practically overridden/negotiated values, so neither of them will be framed (split).

Not to mention the frame_max minimum (the initial frame_max in core team parlance)
used by RabbitMQ for the connection negotiation stage before authentication and authorization. Sometimes it matters a great deal for client libraries, see #787.

But the claim that "RabbitMQ does not respect the negotiated frame_max" is incorrect and likely means that the person making such a claim does not understand what frame_max does exactly, or how
all the protocols RabbitMQ supports rely on/do not reinvent TCP and its segmenting.

[ohroy]

@michaelklishin
Thank you for your detailed explanation; I learned a lot from it. Out of respect for the rabbitmq server, I want to clarify that my intention is not related to OFFICE RABBITMQ SERVER, and in fact, I clearly stated:

Finally, I found the reason is rabbit from alibaba cloud, when use local docker version rabbitmq, it works....

However, in reality, some cloud server vendors do not necessarily comply with this specification for cost reasons, ....... whatever you set frame max, the server never spilt frame, even they said frame size 0x8000 when tune.

I clearly pointed out that the official version of rabbitmq-server follows negotiated specifications, but third-party cloud service providers do not necessarily adhere to this.

Once again, I emphasize that even without this patch, the combination of the library and official RabbitMQ is completely fine. The issue arises only with third-party cloud service vendors' specific versions of the claimed RabbitMQ server.

[michaelklishin]

@ohroy fair enough!