Skip to content

Fix memory leaks in parameter API

Derek Buitenhuis requested to merge dwbuiten/x264:api_leak_fix into master

So this MR has two parts, and feel free to ask for them to be split, or to review them separately — I suspect the second may be more "controversial" than the first. I consider the first much more important than the second, here.

Change 1 (commit message):

API: Add x264_param_cleanup()

Currently, x264_param_parse() calls strdup() to populate any member of
x264_param_t that is a string, and this leaks on exit. This is a small
amount on its own, but can compound over time, if, for example, a long
running application creates and destroys many encoders over its lifetime.

To fix this, we add a new API call that frees these members, but not
the x264_param_t itself, which may exist on the stack. The reason we
need an API call, instead of just having the user free these fields,
is because we allocate these fields inside libx264, and our strdup()
may mismtach with the user's free().

One change that must be made to accomodate this new API, is that
x264_param_default() must allocate the default stats filename on
the heap, instead of the stack, since we try to free it later.
This is a little awkward, since x264_param_default() is a void
type.

I think this one should be pretty straight forward. The only bit that is a little iffy is the strdup failure path in x264_param_default(). I'm also not sure if the function name is the best it could be, so comments welcome.

Change 2 (commit message):

API: Properly handle allocation failures in x264_param_parse()

Check to make sure no call to strdup() failed to allocate.

A new return code, X264_PARAM_ALLOC_FAILED, which is consistent
with the existing documentation, has been added to accomodate
this.

Further, this also properly handles repeated calls to the
function with the same name, and no longer leaks when this
happens.

This one I am less sure about — mostly whether adding a new return code like this is OK or not.

Currently, each commit bumps X264_BUILD separately, since they each change something in x264.h.

Merge request reports