[vlc-devel] [PATCH] Refactored libvlc_vlm_show_media

[Alexander Bethke]

Hi again,

finally gotten around to it. Sorry for the delay.

Rémi Denis-Courmont wrote:
>
> When asprintf() returns -1, the referenced string pointer is undefined, so you 
> can't blindly free() it.
>
>   
>> +    if( i_success == -1 ) {
>> +        free( psz_response );
>> +        psz_response = strdup( "" );
>> +    }
>> +    return psz_response;
>>     
Ok, I wrapped an if (psz_response) check around that free statement.

>> +        else if ( answer->child ) {
>> +            /* in case everything was requested  */
>> +            if ( strcmp( psz_name, "" ) == 0 )
>> +            {
>> +                psz_fmt = "{\n\t%s\n}\n";
>> +                psz_delimiter = strdup( "\n\t" );
>> +                i_list = 0;
>> +            }
>> +            else
>> +            {
>> +                psz_fmt = "%s\n";
>> +                psz_delimiter = strdup( "\n" );
>> +                i_list = 1;
>> +            }
>>     
> I guess you're leaking psz_delimiter here. You should simply not call 
> strdup().
>
>   
Yep, I removed those. Pretty obvious when looking at it again.

> You are not taking any lock anywhere. Did you check that you were accessing 
> only thread-specific or read-only data? (I don't know VLM very well)
>   
I checked the code as thoroughly as I was able to with my neither 
complete knowledge of vlm and my findings are the following:
The ExecuteCommand routine holds a lock on vlm for the time of executing 
the command, so atomicity is given. The data that's in the supplied 
vlm_message is acquired in vlm_ShowMedia in vlmshell.c. It comes from 
the retrieved vlm_media and the strings are passed to 
vlm_MessageSimpleNew or vlm_MessageNew which, as I understand it, make a 
defensive copy. So as far as I can assess there should be no mutable 
data of the vlm_media be leaking. I am not sure about the vlm design 
principles in that regard though.

Anyway, I gonna send the updated patch, if anybody involved with vlm has 
some objections, please step forward.

Best Regards, Alex