Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
libav: fix memory leak when converting packets to annex-b format
  • Loading branch information
John Törnblom committed Mar 13, 2013
1 parent 819b8c8 commit 84b2264
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions src/muxer/muxer_libav.c
View file
Expand Up @@ -397,6 +397,10 @@ lav_muxer_write_pkt(muxer_t *m, streaming_message_type_t smt, void *data)
return -1;
}

// h264_mp4toannexb filter might allocate new data.
if(packet.data != pktbuf_ptr(pkt->pkt_payload))
av_free(packet.data);

break;
}

Expand Down

14 comments on commit 84b2264

@EricV
Copy link
Contributor

@EricV EricV commented on 84b2264 Mar 13, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt this is correct as you don't check the return value of the call to libav routine. Look at example given in the bug report

@john-tornblom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect, as in still leaking memory?

@EricV
Copy link
Contributor

@EricV EricV commented on 84b2264 Mar 14, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No but doing free when it should not and not checking possible errors...

@john-tornblom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okey, when should the memory be released then?

@EricV
Copy link
Contributor

@EricV EricV commented on 84b2264 Mar 14, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read the code at the line I pointed in the bug!

Pointer change can be done without reallocation if I read the ffmpeg.c code correctly in that case 0 is returned in the function call. >0 mean reallocation occured but that does not mean pointer change in all cases (e.g. malloc chunk has free size left after the returned pointer to store more data)

from realloc documentaion : If the area pointed to was moved, a free(ptr) is done => this means it does not always occur and then the pointer in the pkt should be marked as no more valid!

@john-tornblom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you mean.

The only memory that is released at the line you pointed out (avconv.c:943) is the incomming packet (pkt). In tvheadend, the incomming packet is released elsewhere. However, the data allocated by the filter was not released elsewhere, hence the memory leak. This commit will check if the filter allocated new memory and if so, release it.

@EricV
Copy link
Contributor

@EricV EricV commented on 84b2264 Mar 14, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The right way to know if function reallocated memory is via return value not pointer change. The pointer in the pkt may have become invalid or still be valid depending on the return value. And reading ffmepg.c you will see that pointer change may occur without reallocation.

@john-tornblom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking at http://www.ffmpeg.org/doxygen/trunk/h264__mp4toannexb__bsf_8c_source.html#l00059 three things can happend:

ret == 0 : no filtering was done --> input == output.
ret > 0 : filter was successful --> input != output. (input untouched, output is new allocated memory, not reallocated from input).
ret < 0 : something wen't wrong (ENOMEM, EINVAL etc) --> input == output (see http://www.ffmpeg.org/doxygen/trunk/bitstream__filter_8c_source.html#l00061)

I'm not sure what you mean by "the pointer in the pkt"). the filter doesn't take AVPacket as input. It takes a const uint8* as input and **uint8_t as output. I think you have missunderstood something.

@adamsutton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EricV I'm no expert on this, but having read the code and looked at the documentation from what I can see the input ptr should never be altered, the output ptr can be either newly allocated (from within the filter and may have been realloc'd several times) or will be set to the input if no filtering is required.

So checking in != out will tell you whether memory has been allocated. However the one thing that does concern me about not checking the return value is on error its possible the out ptr has been assigned but subsequently free'd so we may try to double free it. So @john-tornblom I do think you should at least check the return value for the error condition.

Adam

@EricV
Copy link
Contributor

@EricV EricV commented on 84b2264 Mar 14, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adamsutton I thinks, the discussion arise because john is looking directly at the instantiation of av_bitstream_filter_filter for the specific use case while I'm looking to generic usage of av_bitstream_filter_filter in the first place.

When I read http://www.ffmpeg.org/doxygen/trunk/ffmpeg_8c_source.html: line 583

int a = av_bitstream_filter_filter(bsfc, avctx, NULL, &new_pkt.data, &new_pkt.size, pkt->data, pkt->size,pkt->flags & AV_PKT_FLAG_KEY);
if(a == 0 && new_pkt.data != pkt->data && new_pkt.destruct)

I think I can say in general that you can have no reallocation and newtr != ptr when using av_bitstream_filter_filter. And I do disagree with the code for that reason. Not sure for this specific filter instantiation (although trying to understand code in a meeting is not easy) but why not writing code that may indeed serve as a template for transcoding to other format?

I do agree reading the code that ptr pointer shall not be altered.

@john-tornblom
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yepp, return value can be used to check for errors. the question is what to do. close the muxer? drop the package? ignore the error and append package as-is? For now, I'll add a warning message and append package as-is.

@EricV
Copy link
Contributor

@EricV EricV commented on 84b2264 Mar 14, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to test 0 or non zero instead of the pointer for detecting the need to free as well.

@adamsutton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EricV

I understand that you're looking at it from a general point of view. I think we agree that the original ptr cannot be changed, if it were that would be a gross violation and would be out of scope of what can be checked/handled.

I think that the specific line you're talking about a==0 and new != old, is implying that the return ptr has been set to NULL in the event of no filtering (as opposed to set to the input ptr). And therefore the memory needs copying into the original. Though I might have read that wrong.

The only things I can see that needs changing is:

need to check for error return, in which case the return ptr should be assumed free'd and no further action (to cleanup) need be taken. But that the pkt will need to be ignored.

need to check that new ptr has actually been allocated. Which is something not currently done.

Adam

@adamsutton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EricV it all equates to the same thing really, just diff ways of checking. But I do agree that checking the return value makes sense, given that it can indicate the validity of the return ptr.

Please sign in to comment.