New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 7774 link

Starred by 7 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 5696



Sign in to add a comment

Ignoring EOR flag when receiving data from usrsctp, resulting in loss of message integrity for messages between 65KB and 256KB

Project Member Reported by deadbeef@chromium.org, Jun 7 2017

Issue description

What steps will reproduce the problem?
1. Connect two PeerConnections, negotiating a data channel.
2. Send a message with length 65665 bytes.

This should result in a message with 65665 bytes popping out on the other side. But it results in one message of size 65664 bytes and another of size 1 bytes. The issue is explained by Lennart Grahl on this blog post: https://lgrahl.de/articles/demystifying-webrtc-dc-size-limit.html

"On the receiver side, usrsctp buffers the small chunks until the buffer reaches the partial delivery point (which is ~64 KiB at default). It then passes the still incomplete message to the browser. Chromium is supposed to buffer the message and wait for the EOR (end of record) flag which indicates that a message is complete. However, Chromium ignores that flag and simply delivers the incomplete message to the user application, violating the message-oriented principle."

So, it sounds pretty clear what needs to be done here.
 
Project Member

Comment 1 by deadbeef@chromium.org, Jun 7 2017

Description: Show this description
Project Member

Comment 2 by deadbeef@chromium.org, Jun 7 2017

Summary: Ignoring EOR flag when receiving data from usrsctp, resulting in loss of message integrity for messages between 65KB and 256KB (was: Ignoring EOR flag when receiving data from SCTP, resulting in loss of message integrity for messages between 65K and 256KB)
A few more comments from my side:

We're actively working on a fix for this for Mozilla Firefox, see this ticket: https://bugzilla.mozilla.org/show_bug.cgi?id=1335262
In Firefox, there's a real backwards compatibility issue we need to resolve that Chrome doesn't have (because you've never implemented PPID-based fragmentation/reassembly although you handle the PPIDs). Once this has been resolved, you should also strongly consider removing use of those PPIDs from your implementation. (Let me know if this should be in a separate issue.)

Furthermore, if you want to test against another data channel implementation that uses explicit EOR for sending and handles EOR when receiving (or you want to have a look how we've done it), check out the patches submitted to Bugzilla (WIP) or the RAWRTC data channel implementation: https://github.com/rawrtc/rawrtc/blob/master/src/librawrtc/sctp_transport.c
Wrong bugzilla ticket, here's the correct one: https://bugzilla.mozilla.org/show_bug.cgi?id=979417
Project Member

Comment 5 by deadbeef@chromium.org, Jun 7 2017

Description: Show this description
Project Member

Comment 6 by deadbeef@chromium.org, Jun 7 2017

Cc: lennart....@gmail.com
Could you explain more about why we should strongly consider removing PPIDs? And yeah, file a new issue if it makes sense.

Also, if I understand correctly: using explicit EOR would allow us to send messages larger than 256KB, while paying attention to the received EOR flag will allow us to receive messages larger than 64KB (which is what this bug is mostly talking about). Correct?
Correct. Explicit EOR mode is relevant for sending only. It makes calls to usrsctp_sendv non-atomic but allows to send messages of arbitrary size. Indeed, this issue is not about explicit EOR for sending. I only wanted to mention that both Firefox (soon) and RAWRTC use explicit EOR to send large messages which you can use for testing purposes (reassembling large messages from Firefox/RAWRTC by looking at the EOR flag).

But of course, it would be great if Chrome would also support sending larger messages than 256 KiB by using explicit EOR.

I'll open a new issue for the deprecated PPIDs discussion.
Project Member

Comment 8 by anatolid@chromium.org, Jan 22 2018

[bulk-edit] This bug is in status Available and has an Owner... This should be corrected. Can the owner please remove themselves from the bug if they are not working on it to make it truly Available, or change the status to reflect the actual state of the bug?
Project Member

Comment 9 by deadbeef@chromium.org, Mar 13 2018

Blocking: 5696
Project Member

Comment 10 by hta@webrtc.org, Apr 26 2018

 Issue 8419  has been merged into this issue.
Project Member

Comment 11 by jeroe...@webrtc.org, Jul 17

Owner: jeroe...@webrtc.org
Status: Assigned (was: Available)
It's good to see progress on this. :) A few comments on the changes:

Be aware, the code is not ready for ndata where stream IDs can be interleaved on receiving.

It doesn't look like this addresses the issue on sending, does it? I would be surprised if sending more than 256 KiB worked with this.

Once your code is ready for ndata, it would be nice if you would also reassemble in case the deprecated "partial message" PPIDs are being used. It's an extremely cheap fix, see: https://github.com/rawrtc/rawrtc/blob/master/src/librawrtc/sctp_transport.c#L1342:L1345

Last but not least:

>      if (!(flags & MSG_EOR) &&
>          (transport->partial_message_.size() < kSendBufferSize)) {
>        return 1;
>      }

Please don't do that. I'm not asking you to go batshit insane like we did for Firefox (2 GiB) but there's room for much more than 256 KiB. Don't you have a send queue size of 8 MiB or 16 MiB? I'd say that would be a reasonable size to start with.

Cheers
Lennart
Project Member

Comment 15 by jeroe...@webrtc.org, Jul 20

Lennart,

You have any idea when ndata support will land in usrsctp? I'm definitely interested in supporting it.

Once we have ndata (and avoid the issue of head-of-line blocking by large messages), we can reconsider the message sizes.

It seems that to leverage this from browser app, we'd also want to enable sending and receiving them in chunks (e.g. enhance the JavaScript APIs for DataChannel messages). Reading in an 8MB file and sending it as a single message isn't very appealing.

It'd be great if you can create an account on Gerrit, so I can add you as a reviewer on related changes.

Thanks,
Jeroen
I've asked Michael regarding ndata support in usrsctp. Will keep you informed.

Yes, we need API changes to leverage large messages efficiently. A streams extension has been proposed: https://github.com/w3c/webrtc-pc/issues/1732
We'll start working on this as part of an extension spec soon.

However, I disagree that the amount of data that can be sent/received in a single message needs to be this low. Chrome allows for 1 MiB web socket messages, so I think it should be at least that. Since the APIs are very similar and I know of use cases who use data channels if possible and web socket as fallback, matching the limitations would be a good idea IMO. (But FWIW the web socket folks are also very interested in adopting our streams extension once we have it for data channels.)

Thought I had an account on Gerrit, mh. Well, it looks like I have one now.
Response from Michael: No ETA for ndata in usrsctp but its fairly advanced.

Cheers
Lennart
Project Member

Comment 18 by jeroe...@webrtc.org, Aug 17

Status: Fixed (was: Assigned)
Okay, from a technical perspective this fixes the EOR bug but it doesn't fix the main problem we have: Violation of message integrity. IIRC, messages are still delivered in chunks in case the message is larger than 256 KiB.
Project Member

Comment 20 by jeroe...@webrtc.org, Aug 17

Unless there is support for some form of streaming, a message limit will always exist. 

I'll create separate issues for supporting sctp ndata and the streams extension (when available).
It's fine to wait for ndata before this implementation allows for sending larger messages. I'd even understand if you wait for the streams extension before allowing larger messages to be sent. No hard feelings.

On the receiver side however, the situation is different (and does not depend on whether ndata is supported or not). If the peer receives a message that is really large, it must either eat it (and by that I mean reassemble and hand it out as an event once it is EOR) or panic, throw an error and close the channel. Panicking is legitimate iff the remote peer did not obey the message size limitation that has been sent as part of the SDP (a=max-message-size which Chrome sadly doesn't send, so the remote must assume the limit is 64 KiB which is the default). But what's really important is that not panicking, and thereby violating message integrity, is not legitimate.
Project Member

Comment 22 by jeroe...@webrtc.org, Aug 17

I see your point.

We need explicit semantics around message sizes (SDP attribute + violation handling). How would a web developer know what this message limit is?
The limit can be fetched from peerConnection.sctp.maxMessageSize (see: https://w3c.github.io/webrtc-pc/#dom-rtcsctptransport-maxmessagesize). But sadly none of the browsers implement that object at the moment. The good news is that we have implemented it in adapter.js and here's an example for it: https://lgrahl.de/examples/dc/dc-test-mms.html (see lines 283+)
I think this should be reopened since the violation still exists.
Yeah... thanks for taking me seriously.
Project Member

Comment 26 by jeroe...@webrtc.org, Oct 14

Status: Assigned (was: Fixed)
Sorry, I am taking you seriously and planned on creating seperate bugs for
1 communicating message limits in SDP
2 support the SCTPTransport MaxMessageSize property
3 support n-data (exists already I think)
4 support streaming interface

It's great that support for the MaxMessageSize has been added to adapter.js, but since the data-channel implementation is unaware of that. E.g. how do we know the recipients message limit?

I think a proper implementation would need at least 1 and 2, and to support xl messages also 3 and 4.

I'll reopen this so it stays on my radar.
The planned issues sound good. But a fundamental one is missing which is the one about reassembling messages or alternatively panic as explained in comment #21. This means either raising the `error` event or closing the channel... or both. I would prefer both.

> It's great that support for the MaxMessageSize has been added to adapter.js, but since the data-channel implementation is unaware of that. E.g. how do we know the recipients message limit?

Who is "we" in this case? :)

The application knows it if it uses adapter.js. An attempt to send more than the announced maximum message size will result in an exception (again, shimmed by adapter.js).

adapter.js knows it because it introspects the received SDP. JSEP specifies to assume the other peer can receive 64 KiB messages if `max-message-size` is not set in the SDP. And that works for all existing implementations I know of.

The stack could do the same as adapter.js.
Supplement: This issue's title perfectly fits the problem described in #21, so I guess it could simply be left open for that.

Sign in to add a comment