[BUG] Access to freed memory when rtpPacket is discarded

[wreuven]

1) In PeerConnection.c, jitterBufferPush(...) is called with pRtpPacket.
2) jitterBufferPush may choose to discard the packet (i.e. free the memory of the RTP packet) 3) the last line quoted below accesses pRtpPacket without regard to the fact that it may no longer exist

4) possible fix is to move the first 4 lines till after access to the packet (there is also another such access a little later)

        CHK_STATUS(jitterBufferPush(pTransceiver->pJitterBuffer, pRtpPacket, &discarded));
        if (discarded) {
            packetsDiscarded++;
        }
        lastPacketReceivedTimestamp = KVS_CONVERT_TIMESCALE(now, HUNDREDS_OF_NANOS_IN_A_SECOND, 1000);
       headerBytesReceived += RTP_HEADER_LEN(pRtpPacket);

[jdelapla]

Hello @wreuven ,

Thanks for bringing this to our attention. I took a closer look at this behavior, and it turns out there is no bug here, though the naming conventions could be better.

in jitterBufferPush() when the packet is discarded, it discards the byte array of the packet. The function used for this is freeRtpPacket(), and it safely frees the memory of pRtpPacket->pRawPacket, not the entire struct at pRtpPacket. The rest of the struct's members remain unchanged.

in PeerConnection.c where jitterBufferPush() was called, there is a variable pPayload, which is used to 'create' the RTP packet, by assigning pPayload to pRtpPacket->pRawPacket. To avoid releasing this memory twice, there is a bool 2 lines below where your quote ends: ownedByJitterBuffer = TRUE

This bool is used in the CleanUp section to ensure the memory is not freed again.

I'll be closing this issue, if my answer isn't satisfactory, then please reach out and we can continue the conversation.

Thanks.

Edit: Bolded all function and variable names for formatting purposes. Fixed grammar

[wreuven]

Please note the second free hilighted in bold. This one frees the entire packet.

STATUS freeRtpPacket(PRtpPacket* ppRtpPacket) { ENTERS();

STATUS retStatus = STATUS_SUCCESS;

CHK(ppRtpPacket != NULL, STATUS_NULL_ARG);

if (*ppRtpPacket != NULL) {
    SAFE_MEMFREE((*ppRtpPacket)->pRawPacket);
}
*SAFE_MEMFREE(*ppRtpPacket)*;

CleanUp:

CHK_LOG_ERR(retStatus);

LEAVES();
return retStatus;

}

On Wed, May 5, 2021, 12:56 AM jdelapla @.***> wrote:

Hello @wreuven https://github.com/wreuven ,

Thanks for bringing this to our attention. I took a closer look at this behavior, and it turns out there is no bug here, though the naming conventions could be better.

in jitterBufferPush() when the packet is discarded, it discards the byte array of the packet. The function used for this is freeRtpPacket(), and it safely frees the memory of pRtpPacket->pRawPacket, not the entire struct pRtpPacket. The rest of the struct's members remain unchanged.

in PeerConnection.c where jitterBufferPush() was called, there is a variable pPayload, which is used to 'create' the RTP packet, but assigned pPayload to pRtpPacket->pRawPacket. To avoid releasing this memory twice, there is a bool 2 lines below where your quote ends: ownedByJitterBuffer = TRUE

This bool is used in the CleanUp section to ensure the memory is not freed again.

I'll be closing this issue, if my answer isn't satisfactory, then please reach out and we can continue the conversation.

Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/issues/1132#issuecomment-832276640, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPUZQXIEHZRDT5NRZW27SLTMBUQVANCNFSM43XQ3LEQ .

[MushMal]

These are separate allocations

[wreuven]

Please read my original bug report and the response by jdelapla. Jdelapla writes that only the first free happens and therefore the rtppacket "metadata" can still be accessed after rtpfreepacket. Here we see that both the data and metadata are freed.

On Thu, May 6, 2021, 12:51 AM Mushegh Malkhasyan @.***> wrote:

These are separate allocations

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/issues/1132#issuecomment-833053997, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPUZQSBMCTEOCIRPDMCFUDTMG4XVANCNFSM43XQ3LEQ .

[MushMal]

@wreuven apologies, you are correct. There is a potential of accessing a freed memory from the PeerConnection. This needs to be properly fixed. Your suggestion of moving the accessor lines to above code might be correct but someone needs to take a closer look whether there needs to be an else clause instead after the pushRtpPacket call instead.

[MushMal]

@jdelapla @disa6302 could you re-open the issue please?

[disa6302]

PR merged. Thanks for reporting the bug. Resolving.