Fix SIGSEGV at RTC::WebRtcTransport::OnIceServerTupleRemoved()

[ibc]

• Fixes SIGSEGV at 'RTC::WebRtcTransport::OnIceServerTupleRemoved' #883

Details

As @ybybwdwd said in in #883 (comment):

• IceServer manages a list of TransportTuples instances in a std::list. Those are not pointers.
• When IceServer removes a tuple from the list (for different reasons), it removes it from its std::list, which means that the removed TransportTuple is freed. And then it notifies WebRtcTransport via this->listener->OnIceServerTupleRemoved(this, removedTuple);.
• Problem was that WebRtcTransport reads tuple data (tuple->GetProtocol() etc) within its OnIceServerTupleRemoved() method, so it's reading freed memory!, so crash.

Solution is to first notify the listener (WebRtcTransport) then remove it from the list (so free it after used).

[jmillan]

Apart from a range loop change suggestion, 👍

[jmillan]

@ibc,

I find this cleaner:

diff --git a/worker/src/RTC/IceServer.cpp b/worker/src/RTC/IceServer.cpp
index 27db36344..5962a4516 100644
--- a/worker/src/RTC/IceServer.cpp
+++ b/worker/src/RTC/IceServer.cpp
@@ -294,6 +294,8 @@ namespace RTC
                if (!removedTuple)
                        return;

+               // Notify the listener.
+               this->listener->OnIceServerTupleRemoved(this, removedTuple);
                // Remove it from the list of tuples.
                this->tuples.erase(it);

@@ -316,9 +318,6 @@ namespace RTC
                                this->listener->OnIceServerDisconnected(this);
                        }
                }
-
-               // Notify the listener.
-               this->listener->OnIceServerTupleRemoved(this, removedTuple);
        }

        void IceServer::ForceSelectedTuple(const RTC::TransportTuple* tuple)

We first notify the listener about the removed tuple, then tell it about the new selected one or about the disconnection. This flow seems more logical to me.

[ibc]

Agreed. Please @jmillan review again.

[jmillan]

👍