Closed Bug 861958 Opened 7 years ago Closed 7 years ago

DataChannelConnection::Open() needs to auto-extend streams if needed by a pre-negotiated stream

Categories

(Core :: WebRTC: Networking, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox21 --- disabled
firefox22 + fixed
firefox23 --- verified

People

(Reporter: posidron, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [webrtc][blocking-webrtc+])

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file testcase
This crash occurred while fuzzing the DataChannel API with the JSEP fuzzer.

Tested with m-i changeset: 128765:53c2e7b9753b
Attached file callstack
This uses a pre-negotiated stream outside the bounds of the negotiated max stream value.  The code needs to negotiate more space to include this pre-negotiated stream.
Assignee: nobody → rjesup
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86_64 → All
Summary: WebRTC assertion failure: i < Length() (invalid array index) and crash [@mozilla::DataChannelConnection::Open] → DataChannelConnection::Open() needs to auto-extend streams if needed by a pre-negotiated stream
Whiteboard: [WebRTC][blocking-webrtc+]
Perhaps we should be using mStream.SafeElementAt(aStream) instead of mStreams[aStream] on line 1835 of DataChannel.cpp as well.
I'm also updating the datachannel test on webrtc_landing to allow testing this
Whiteboard: [WebRTC][blocking-webrtc+] → [webrtc][blocking-webrtc+][webrtc-uplift]
Attachment #737622 - Flags: review?(tuexen)
Comment on attachment 737622 [details] [diff] [review]
Support pre-negotiated streams greater than the currently-allocated number

Review of attachment 737622 [details] [diff] [review]:
-----------------------------------------------------------------

I think both of my comments need to be addressed.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +1895,5 @@
>        stream = FindFreeStream(); // may be INVALID_STREAM if we need more
> +    }
> +    if (stream == INVALID_STREAM || stream >= mStreams.Length()) {
> +      // RequestMoreStreams() limits to MAX_NUM_STREAMS
> +      int32_t needed = (stream == INVALID_STREAM) ? 16 : (std::max(16, stream+16));

In case of stream == INVALID_STREAM you are requesting 16
more streams. OK.
But in the other case you are requesting a possible huge
number of streams. Why?
needed is the number of streams to add here.

@@ +1909,5 @@
> +        mState == CLOSED) {
> +      // Update number of streams for init message
> +      struct sctp_initmsg initmsg;
> +      socklen_t len = sizeof(initmsg);
> +      int32_t needed = (stream == INVALID_STREAM) ? 16 : (std::max(16, stream+16));

needed here is the number of streams, not the number of streams to add. So it is different from the above.
I don't think the formula makes sense here.
Attachment #737622 - Attachment is obsolete: true
Attachment #737622 - Flags: review?(tuexen)
Comment on attachment 738292 [details] [diff] [review]
Support pre-negotiated streams greater than the currently-allocated number

Revised to correct the needed values, and renamed more_needed and total_needed to remove confusion.
Attachment #738292 - Flags: review?(tuexen)
Comment on attachment 738292 [details] [diff] [review]
Support pre-negotiated streams greater than the currently-allocated number

Review of attachment 738292 [details] [diff] [review]:
-----------------------------------------------------------------

I think you want something like I wrote. If that is correct, you can change the state to review+

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +1896,5 @@
> +    }
> +    if (stream == INVALID_STREAM || stream >= mStreams.Length()) {
> +      // RequestMoreStreams() limits to MAX_NUM_STREAMS
> +      int32_t more_needed = (stream == INVALID_STREAM) ? 16 :
> +                            (std::max(16, (stream-((int32_t)mStreams.Length())) + 16));

I think you want something like
int32_t more_needed = (stream == INVALID_STREAM) ? 16 :
                      std::max(16, (stream - ((int32_t)mStreams.Length())));
if I'm not wrong.
(In reply to Michael Tüxen from comment #8)
> Comment on attachment 738292 [details] [diff] [review]
> Support pre-negotiated streams greater than the currently-allocated number
> 
> Review of attachment 738292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you want something like I wrote. If that is correct, you can change
> the state to review+
> 
> ::: netwerk/sctp/datachannel/DataChannel.cpp
> @@ +1896,5 @@
> > +    }
> > +    if (stream == INVALID_STREAM || stream >= mStreams.Length()) {
> > +      // RequestMoreStreams() limits to MAX_NUM_STREAMS
> > +      int32_t more_needed = (stream == INVALID_STREAM) ? 16 :
> > +                            (std::max(16, (stream-((int32_t)mStreams.Length())) + 16));
> 
> I think you want something like
> int32_t more_needed = (stream == INVALID_STREAM) ? 16 :
>                       std::max(16, (stream - ((int32_t)mStreams.Length())));
> if I'm not wrong.

Actually, the +16 in the final case is intentional - I purposely decided if I'm going to extend it, I want to include extra channels past the 'target' one, to avoid one extra trip through RequestMoreStreams() in a possibly common case of in-order creation:
(comments are if you don't have the +16)

   createDataChannel("",{stream:50}); // RequestMoreStreams extends to 50
   createDataChannel("",{stream:51}); // queues, and after we get 50 streams
                                      // RequestMoreStreams to 51+16
   createDataChannel("",{stream:52});
(In reply to Randell Jesup [:jesup] from comment #9)
> (In reply to Michael Tüxen from comment #8)
> > Comment on attachment 738292 [details] [diff] [review]
> > Support pre-negotiated streams greater than the currently-allocated number
> > 
> > Review of attachment 738292 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I think you want something like I wrote. If that is correct, you can change
> > the state to review+
> > 
> > ::: netwerk/sctp/datachannel/DataChannel.cpp
> > @@ +1896,5 @@
> > > +    }
> > > +    if (stream == INVALID_STREAM || stream >= mStreams.Length()) {
> > > +      // RequestMoreStreams() limits to MAX_NUM_STREAMS
> > > +      int32_t more_needed = (stream == INVALID_STREAM) ? 16 :
> > > +                            (std::max(16, (stream-((int32_t)mStreams.Length())) + 16));
> > 
> > I think you want something like
> > int32_t more_needed = (stream == INVALID_STREAM) ? 16 :
> >                       std::max(16, (stream - ((int32_t)mStreams.Length())));
> > if I'm not wrong.
> 
> Actually, the +16 in the final case is intentional - I purposely decided if
> I'm going to extend it, I want to include extra channels past the 'target'
> one, to avoid one extra trip through RequestMoreStreams() in a possibly
> common case of in-order creation:
> (comments are if you don't have the +16)
> 
>    createDataChannel("",{stream:50}); // RequestMoreStreams extends to 50
>    createDataChannel("",{stream:51}); // queues, and after we get 50 streams
>                                       // RequestMoreStreams to 51+16
>    createDataChannel("",{stream:52});

OK. Understood.
Then you want
stream-((int32_t)mStreams.Length()) + 16
instead of
(std::max(16, (stream-((int32_t)mStreams.Length())) + 16))
(In reply to Michael Tüxen from comment #10)

> OK. Understood.
> Then you want
> stream-((int32_t)mStreams.Length()) + 16
> instead of
> (std::max(16, (stream-((int32_t)mStreams.Length())) + 16))

Sure.  Will mark r+ with that adjustment
Comment on attachment 738292 [details] [diff] [review]
Support pre-negotiated streams greater than the currently-allocated number

r=tuexen
Attachment #738292 - Flags: review?(tuexen) → review+
(In reply to Randell Jesup [:jesup] from comment #11)
> (In reply to Michael Tüxen from comment #10)
> 
> > OK. Understood.
> > Then you want
> > stream-((int32_t)mStreams.Length()) + 16
> > instead of
> > (std::max(16, (stream-((int32_t)mStreams.Length())) + 16))
> 
> Sure.  Will mark r+ with that adjustment

Thanks!
https://hg.mozilla.org/mozilla-central/rev/7f9636b8376f
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Keywords: verifyme
Comment on attachment 738292 [details] [diff] [review]
Support pre-negotiated streams greater than the currently-allocated number

[Approval Request Comment]
Bug caused by (feature/regressing bug #): support for pre-negotiation of channels - bug 855623

User impact if declined: Can only negotiate channels up to the current max allocated (16 to start), which is not directly visible to the application.

Testing completed (on m-c, etc.): on m-c, waiting on verifyme.  Tested with multiple tests and updated the webrtc-landing data channel test to allow selecting any channel number.

Risk to taking this patch (and alternatives if risky): low, and pre-negotiation is a new feature needed by the IETF changes agreed to in Orlando.  Workaround is to use less than stream 16 always (since we haven't implemented setting the default number of streams yet).

String or IDL/UUID changes made by this patch: none
Attachment #738292 - Flags: approval-mozilla-aurora?
Ran the attached test case a bunch of times, no crashes seen on trunk.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Attachment #738292 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3cd536bb8c6
Whiteboard: [webrtc][blocking-webrtc+][webrtc-uplift] → [webrtc][blocking-webrtc+]
Attachment #744433 - Flags: review?(rjesup)
Attachment #744433 - Flags: review?(rjesup) → review+
You need to log in before you can comment on or make changes to this bug.