Comment # 7
on bug 9920
from Hadriel Kaplan
(In reply to comment #6)
> but the INVITE's SDP has two m= lines for the same port number, so my guess
> is RTP is deleting the hash table of the first media stream when it's told
> to add the second one, because they'll be in the same ip:port conversation.
> But SDP will think the first one is valid still. And then the 200 OK's SDP
> disables the second one, so RTP will set the conversation hashtable back to
> the one it previously free'd.
Yeah, I enabled the debug printing stuff and that's what's happening.
In fact there's another bug happening too, but it's not causing the crash. The
unrelated bug is that in the 200 OK, the SDP dissector is inserting the dynamic
payload hash for the rtpmap into the wrong media channel. It should be
inserting it into channel #2, but it's inserting it for channel #3. I'll open a
different bug for that one.
But getting back to this bug... there are really two problems:
1) The memory ownership model for the rtp_dyn_payload hashtable is split: SDP
creates the rtp_dyn_payload hashtable, but RTP can free it. Since there isn't
*one* pointer to the hashtable, RTP freeing it means SDP has a dangling
pointer. Or another way to think about it: SDP calls rtp_add_address() with its
rtp_dyn_payload pointer *value*, not a pointer to the pointer; so RTP is
operating on a local pointer which it copies and later free's, and SDP will
never know. Fundamentally this isn't safe - it's what causes these types of
crashes, and makes it hard to debug because the crashing isn't consistently
reproducible because a free'd hashtable memory location might still be valid
content. (in fact, I have a very hard time reproducing this bug - it's a roll
of the dice)
2) Either the SDP dissector shouldn't be creating two separate, unique
hashtables for multiple media channels of the same addr:port, or RTP shouldn't
be free'ing the previous one. The latter change would be tricky, because RTP
would have to figure out what to do with them: i.e., should it keep both
somehow? Also, RTP does need to truly replace the hashtable sometimes; for
example when a new SDP offer/answer round changes the info for the same RTP
ip:port, or even if it's just a new call that happens to use the same RTP
addr:port as a previously ended call. (although what if the previous call
didn't end? Then you could get this crash again due to issue (1) above)
Historically its been ambiguous whether having two or more m= lines for the
same address and port number is legitimate - we've debated this issue in the
IETF numerous times. Lately, however, there has been a move towards allowing
it; mostly for WebRTC, but potentially for SIP too. So I think the right fix
for issue (2) above is to make SDP use the same hashtable for media channels of
the same addr:port.
The right fix for issue (1) is to stop using just local pointer values.
Unfortunately SDP is not the only dissector to setup RTP flows. There about a
dozen others.
You are receiving this mail because:
- You are watching all bug changes.
