[x264-devel] [Git][videolan/x264][master] Add new API function: x264_param_cleanup

[BugMaster]

On Mon, 6 Jul 2020 09:23:59 +0200, Alexander Strasser wrote:
> [...]

>> +char *x264_strdup( x264_param_t *param, const char *src )

> IMHO the naming is a bit unfortunate.

> Wouldn't x264_param_strdup or similar have been a little bit better?

> If I read x264_strdup I think it's an implementation of the well known
> strdup function in x264's code base. Therefore I would expect the same
> function signature as strdup usually has.

I am not sure about this (I don't have such expections when I see it).

> If I understood correctly, this function is not considered public API
> and therefore could still be renamed if you agree another name would
> be better.

Yes, renaming is possible if we decide that another name would be more
appropriate for it.

>> +{
>> +    strdup_buffer *buf = param->opaque;
>> +    if( !buf )
>> +    {
>> +        buf = malloc( BUFFER_OFFSET + BUFFER_DEFAULT_SIZE * sizeof(void *) );
>> +        if( !buf )
>> +            goto fail;
>> +        buf->size = BUFFER_DEFAULT_SIZE;
>> +        buf->count = 0;
>> +        param->opaque = buf;
>> +    }
>> +    else if( buf->count == buf->size )
>> +    {
>> +        if( buf->size > (INT_MAX - BUFFER_OFFSET) / 2 / (int)sizeof(void *) )
>> +            goto fail;
>> +        int new_size = buf->size * 2;
>> +        buf = realloc( buf, BUFFER_OFFSET + new_size * sizeof(void *) );

> This will leak memory if realloc fails.

> I guess it wouldn't be a big problem in typical usage scenarios, but
> probably better to just handle it correctly.

I don't see any memory leaks here. And result of realloc correctly
checked for NULL later.

>> +        if( !buf )
>> +            goto fail;
>> +        buf->size = new_size;
>> +        param->opaque = buf;
>> +    }
>> +    char *res = strdup( src );
>> +    if( !res )
>> +        goto fail;
>> +    buf->ptr[buf->count++] = res;
>> +    return res;
>> +fail:
>> +    x264_log_internal( X264_LOG_ERROR, "strdup failed\n" );

> Maybe changing the log to "x264_strdup failed\n" is more precise.

Probably yes. Especially if we decide to rename this function.