Fix IceServer crash when client uses ICE renomination

[ibc]

Fixes #1176

Details

• Problem was in this PR 756 that added support for ICE renomination: https://datatracker.ietf.org/doc/html/draft-thatcher-ice-renomination
• Before that PR, we always assumed that if there is a tuple then we are in 'connected' or 'completed' state, so there is a selected ICE tuple.
• That's no longer true when client uses ICE nomination stuff.

[ibc]

@penguinol could you please take a look to this?

[penguinol]

Maybe we do not need to remove MS_ASSERT in IceState::NEW and IceState::DISCONNECTED, just reset this->remoteNomination = 0 when state change to IceState::DISCONNECTED is enough.

[ibc]

Maybe we do not need to remove MS_ASSERT in IceState::NEW and IceState::DISCONNECTED, just reset this->remoteNomination = 0 when state change to IceState::DISCONNECTED is enough.

I don't think so. In the original code, when HandleTuple() is called, we first assert that tuple is empty and later we check nomination, but the assertion already happened and there were tuples so it aborted the process.

The thing here is that, before introducing renomination stuff, IceServer logic assumed that if there is any tuple then state was connected or completed and there was a selectedTuple. This is no longer true. With renomination it can happen that the first time we call to HandleTuple() the tuple is added to the list of tuples but selectedTuple remains unset and state remains new (or disconnected).

So I have a question. Scenario:

• State is new with no tuples.
• HandleTuple() is called for first time with a candidate with nomination 0.
• In that case the tuple is added to the tuple list but it doesn't become selectedTuple and state remains NEW.

Is this correct? Or should state change to connected and the tuple become the selectedTuple?

[jmillan]

Looks good 👍

[penguinol]

Maybe we do not need to remove MS_ASSERT in IceState::NEW and IceState::DISCONNECTED, just reset this->remoteNomination = 0 when state change to IceState::DISCONNECTED is enough.

I don't think so. In the original code, when HandleTuple() is called, we first assert that tuple is empty and later we check nomination, but the assertion already happened and there were tuples so it aborted the process.

The thing here is that, before introducing renomination stuff, IceServer logic assumed that if there is any tuple then state was connected or completed and there was a selectedTuple. This is no longer true. With renomination it can happen that the first time we call to HandleTuple() the tuple is added to the list of tuples but selectedTuple remains unset and state remains new (or disconnected).

So I have a question. Scenario:

• State is new with no tuples.
• HandleTuple() is called for first time with a candidate with nomination 0.
• In that case the tuple is added to the tuple list but it doesn't become selectedTuple and state remains NEW.

Is this correct? Or should state change to connected and the tuple become the selectedTuple?

According to https://webrtc.googlesource.com/src/+/refs/heads/main/p2p/base/connection.cc#1037
Nomination will not be 0

[ibc]

According to https://webrtc.googlesource.com/src/+/refs/heads/main/p2p/base/connection.cc#1037
Nomination will not be 0

Yes, but we cannot crash if client sends a STUN binding request with nomination 0.

[ibc]

@jmillan commented in #756 (review) about those conditions in HandleTuple(). My response here:

Those conditions are basically like this:

case IceState::NEW:
{
	if (!hasUseCandidate && !hasNomination)
	{
		// Store the tuple.
		auto* storedTuple = AddTuple(tuple);

		// Mark it as selected tuple.
		SetSelectedTuple(storedTuple);

		// Update state.
		this->state = IceState::CONNECTED;

		// Notify the listener.
		this->listener->OnIceServerConnected(this);
	}
	else
	{
		// Store the tuple.
		auto* storedTuple = AddTuple(tuple);

		if ((hasNomination && nomination > this->remoteNomination) || !hasNomination)
		{

			// Mark it as selected tuple.
			SetSelectedTuple(storedTuple);

			// Update state.
			this->state = IceState::COMPLETED;

			// Update nomination.
			if (hasNomination && nomination > this->remoteNomination)
			{
				this->remoteNomination = nomination;
			}

			// Notify the listener.
			this->listener->OnIceServerCompleted(this);
		}
	}

	break;
}

@jmillan said:

The condition previous to this else block assures that here hasUseCandidate is true and hasNominamtion is also true, so checking that again here is not needed, and makes it harder to read. I had to go to the previous block to make sure this conditions were repeated.

But that's not true:

• The fist condition is if (!hasUseCandidate && !hasNomination).
• So if we are in the else block it means that hasUseCandidate OR hasNomination is true (or both).

[jmillan]

The fist condition is if (!hasUseCandidate && !hasNomination).
So if we are in the else blockit means thathasUseCandidateORhasNomination` is true (or both).

Totally true 👍