1303867 - Missing DTLS alert on PeerConnection.close()

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P2)

Description

[Nils Ohlmeier [:drno]]

According to a report on dev-media Fx since version 48 no longer sends a DTLS alert message when PeerConnection.close() gets called.

Nils needs to provide the bug which caused this behavior.

Comment 1

[Nils Ohlmeier [:drno]]

^ see description

Comment 2

[Nils Ohlmeier [:drno]]

I'm pretty sure bug 1279146 causes this new behavior. We should check if we can find a better way to have DTLS send a desired DTLS alert on shutdown, but not have it send DTLS packets on already freed sockets.

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: Bug 1303867: destroy SSL FD to send out DTLS allert on close.

Review commit: https://reviewboard.mozilla.org/r/132258/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132258/

Comment 4

[Martin Thomson [:mt:]]

Comment on attachment 8860254 [details]
Bug 1303867: destroy SSL FD to send out DTLS allert on close.

https://reviewboard.mozilla.org/r/132258/#review135156

::: media/mtransport/transportlayerdtls.cpp:373
(Diff revision 1)
>    TransportLayerReserved,
>    TransportLayerReserved
>  };
>  
>  TransportLayerDtls::~TransportLayerDtls() {
> +  ssl_fd_ = nullptr;

So this patch appears to do absolutely nothing.  The destructor of the UniquePRFileDesc should be called either way when the TransportLayerDtls instance is destroyed.  But your concern is ordering, and that's fair.  Note that packet loss will cause the alert to be lost in a few cases anyway.

I think that you need a comment here explaining that the close alert would be otherwise suppressed by disabling the `nspr_io_adapter_` (if that is indeed what is happening).

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8860254 [details]
Bug 1303867: destroy SSL FD to send out DTLS allert on close.

https://reviewboard.mozilla.org/r/132258/#review135156

> So this patch appears to do absolutely nothing.  The destructor of the UniquePRFileDesc should be called either way when the TransportLayerDtls instance is destroyed.  But your concern is ordering, and that's fair.  Note that packet loss will cause the alert to be lost in a few cases anyway.
> 
> I think that you need a comment here explaining that the close alert would be otherwise suppressed by disabling the `nspr_io_adapter_` (if that is indeed what is happening).

Yes this is only about order of destroying so the DTLS alert can still get out. I know it can get lost. But people are complaining that Firefox used to send out the alerts, and now it doesn't any more.

I guess the other option would be instead of disabling the nspr_io_adapter completely, only block reads but still allow write operations from the destructor. But as that would add even more code I decided against it.

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8860254 [details]
Bug 1303867: destroy SSL FD to send out DTLS allert on close.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132258/diff/1-2/

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8860254 [details]
Bug 1303867: destroy SSL FD to send out DTLS allert on close.

https://reviewboard.mozilla.org/r/132258/#review135530

::: media/mtransport/transportlayerdtls.cpp:373
(Diff revision 2)
>    TransportLayerReserved,
>    TransportLayerReserved
>  };
>  
>  TransportLayerDtls::~TransportLayerDtls() {
> +  // Destroy the NSS instance first so it can still send out an alert before 

trailing space

::: media/mtransport/transportlayerdtls.cpp:374
(Diff revision 2)
>    TransportLayerReserved
>  };
>  
>  TransportLayerDtls::~TransportLayerDtls() {
> +  // Destroy the NSS instance first so it can still send out an alert before 
> +  // we disable the nspr_io_adapter.

missing trailing _

Comment 8

[Nils Ohlmeier [:drno]]

Comment on attachment 8860254 [details]
Bug 1303867: destroy SSL FD to send out DTLS allert on close.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132258/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1979070698f1
destroy SSL FD to send out DTLS allert on close. r=mt

Comment 10

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/1979070698f1

Comment 11

[flaviogrossi]

Is this a problem again?

I can reproduce this again with the latest firefox version (78.0.1) and the nightly version.

I create a Peer connection, then i close it with close(), and i cannot see a dtls alert using wireshark.

Comment 12

[Byron Campen [:bwc]]

Michael, you're working on something related to this, right?

Comment 13

[Michael Froman [:mjf]]

I'm not working on it yet, but :kjacbos ID'd the basic issue a few days ago. SSL3_SendAlert is trying to send the alert, but the stream in NrIceMediaStream::SendPacket is null. This was the trace he gave me showing the issue:

Process 79812 stopped
* thread #10, name = 'Socket Thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000101c298a8 libnss3.dylib`SSL3_SendAlert(ss=0x00000001373fe000, level=alert_warning, desc=close_notify) at ssl3con.c:2869:13 [opt]
   2866	SECStatus
   2867	SSL3_SendAlert(sslSocket *ss, SSL3AlertLevel level, SSL3AlertDescription desc)
   2868	{
-> 2869	    PRUint8 bytes[2];
   2870	    SECStatus rv;
   2871	    PRBool needHsLock = !ssl_HaveSSL3HandshakeLock(ss);
   2872	
Target 0: (firefox) stopped.
(lldb) br set -n NrIceMediaStream::SendPacket
Breakpoint 2: where = XUL`mozilla::NrIceMediaStream::SendPacket(int, unsigned char const*, unsigned long) + 45 at nricemediastream.cpp:621:33, address = 0x0000000102fe2c8d
(lldb) cont
Process 79812 resuming
XUL was compiled with optimization - stepping may behave oddly; variables may not be available.
Process 79812 stopped
* thread #10, name = 'Socket Thread', stop reason = breakpoint 2.1
    frame #0: 0x0000000102fe2c8d XUL`mozilla::NrIceMediaStream::SendPacket(this=0x00000001335fe040, component_id=1, data="/?d, len=24) at nricemediastream.cpp:621:33 [opt]
   618 	
   619 	nsresult NrIceMediaStream::SendPacket(int component_id,
   620 	                                      const unsigned char* data, size_t len) {
-> 621 	  nr_ice_media_stream* stream = old_stream_ ? old_stream_ : stream_;
   622 	  if (!stream) {
   623 	    return NS_ERROR_FAILURE;
   624 	  }
Target 0: (firefox) stopped.
(lldb) n
Process 79812 stopped
* thread #10, name = 'Socket Thread', stop reason = step over
    frame #0: 0x0000000102fe2ca0 XUL`mozilla::NrIceMediaStream::SendPacket(this=0x00000001335fe040, component_id=1, data="/?d, len=24) at nricemediastream.cpp:622:8 [opt]
   619 	nsresult NrIceMediaStream::SendPacket(int component_id,
   620 	                                      const unsigned char* data, size_t len) {
   621 	  nr_ice_media_stream* stream = old_stream_ ? old_stream_ : stream_;
-> 622 	  if (!stream) {
   623 	    return NS_ERROR_FAILURE;
   624 	  }
   625 	
Target 0: (firefox) stopped.
(lldb) p stream
(mozilla::nr_ice_media_stream *) $0 = 0x0000000000000000
(lldb) bt
* thread #10, name = 'Socket Thread', stop reason = step over
  * frame #0: 0x0000000102fe2ca0 XUL`mozilla::NrIceMediaStream::SendPacket(this=0x00000001335fe040, component_id=1, data="/?d, len=24) at nricemediastream.cpp:622:8 [opt]
    frame #1: 0x00000001030078a9 XUL`mozilla::TransportLayerIce::SendPacket(this=0x000000013561d040, packet=0x000070000c612e10) at transportlayerice.cpp:125:27 [opt]
    frame #2: 0x0000000102ff3ae8 XUL`mozilla::TransportLayerNSPRAdapter::Write(this=0x0000000137494bc0, buf=<unavailable>, length=<unavailable>) at transportlayerdtls.cpp:105:32 [opt]
    frame #3: 0x0000000101c4bedb libnss3.dylib`ssl_DefSend(ss=0x00000001373fe000, buf="/?d, len=24, flags=0) at ssldef.c:105:18 [opt]
    frame #4: 0x0000000101c2cc1e libnss3.dylib`ssl3_SendRecord(ss=0x00000001373fe000, cwSpec=0x0000000135621ed0, ct=<unavailable>, pIn=<unavailable>, nIn=<unavailable>, flags=0) at ssl3con.c:2575:20 [opt]
    frame #5: 0x0000000101c29a4e libnss3.dylib`SSL3_SendAlert(ss=0x00000001373fe000, level=alert_warning, desc=close_notify) at ssl3con.c:2903:16 [opt]
    frame #6: 0x0000000101c5260b libnss3.dylib`ssl_SecureClose(ss=0x00000001373fe000) at sslsecur.c:684:15 [opt]
    frame #7: 0x0000000102ff4063 XUL`mozilla::TransportLayerDtls::~TransportLayerDtls() [inlined] mozilla::UniquePRFileDescDeletePolicy::operator(aValue=<unavailable>)(PRFileDesc*) at ScopedNSSTypes.h:301:1 ```

Comment 14

[Martin Thomson [:mt:]]

BTW, we will only send this once, and packets get lost. But it seems like Michael has a handle on where this is failing, so at least we don't fail 100% of the time.

Comment 15

[Michael Froman [:mjf]]

Attachment: Bug 1303867 - teardown Transports before NrIceCtx to allow close_notify alerts. r?bwc!

If the NrIceCtx is torn down first there is not a valid media stream on which
to send the close_notify alert.

Comment 16

[Michael Froman [:mjf]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59fa4412468be6cb1e1efd145ec24cd1f7e3ec9e&selectedTaskRun=FLUTrg9cTfeQJEH9cufMow.0

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/811b3061c123
teardown Transports before NrIceCtx to allow close_notify alerts. r=bwc

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/811b3061c123