Memory access problem in rtpsources.cpp line 339

[lbakman]

Hi Jori

I believe I have found a memory access problem in rtpsources.cpp line 339, if a session implementation overrides OnValidatedRTPPacket, sets ispackethandled to true and deletes the RTPPacket pointer.

In rtpsources.cpp line 232 srcdat->ProcessRTPPacket is called, which in turn calls OnValidatedRTPPacket. In example6 the override deletes the packet using DeletePacket(packet), but in rtpsources.cpp line 339, rtppack->GetCSRCCount() is called on a deleted object and in line rtppack->GetCSRC(i) may be called on the same deleted object.

I do not have a suggestion for a fix, but either the OnValidatedRTPPacket should not delete the packet, or it should be handled in some other way. If OnValidatedRTPPacket deletes the packet, the entire contributing source handling will be broken.

I can just avoid setting ispackethandled to true and delete the packet, but that means that the packets are stored in the packetlist and I am not sure why I would want that (I call Poll() from my own thread as in example8).

Regards
Lau Bakman

[j0r1]

Hi Lau,

You're correct, this is indeed a problem.

I think the easiest fix would be to cache the CSRCs temporarily, before the srcdat->ProcessRTPPacket function is called, and then use those cached values further on (instead of obtaining them from rtppack)

This is what I tried to do in branch 'csrccache' - I haven't tested it though. Could you check if this works for you?

Thanks!
Jori

[lbakman]

Hi Jori

That seems to have fixed the problem when running example 6 with valgrind.

I will do some more testing with my own project today and let you know.

Edit: One thing - if the number of received CSRC's exceed RTP_MAXCSRCS there may be a problem. Perhaps unlikely, but possible. The RTPPacket::ParseRawPacket does not limit the RTPPacket::numcsrcs to RTP_MACCSRCS.

Thank you for the fast response.

Lau

[lbakman]

Hi Jori

I have not found any issues with your fix in the csrccache branch except for the potential problem I mentioned above.

Regards
Lau

[j0r1]

Hi Lau,

The number of CSRCs comes from four bits in the RTP header, so it shouldn't exceed that maximum number. But it's probably best to be extra cautious, so I've added an extra check.

Thanks for testing!

Cheers,
Jori