[vlc-devel] [PATCH 3/3] Initialize title strings with non NULL pointer

[Daniel Glaas]

Hi Thomas,

I really appreciate your patient review, it hopefully made my patch to 
something that contributes to VLC and I also learned a lot about 
handling of chars and char arrays in C.

I attached the corrected version of the patch.

Am 22.07.20 um 08:45 schrieb Thomas Guillem:
> Hello,
>
> Yes the cast is correct but &s_init_title[0] == s_init_title, so you 
> don't need &[0].
>
> On Tue, Jul 21, 2020, at 21:47, Daniel Glaas wrote:
>>
>> Please find attached my reworked commit, I think I implemented all 
>> your remarks.
>>
>> I added some casts to cast away the const qualifier from s_init_title 
>> to avoid that gcc prints warnings about that. I don't know whether 
>> this is a good style in your opinion or not.
>>
>> Am 21.07.20 um 11:20 schrieb Thomas Guillem:
>>> Your commit looks good. Few remarks:
>>>
>>> - You don't need p_initTitle, you can use initTitle directly
>>>
>>> - static char initTitle[] = ""; <- missing const qualifier, the name 
>>> doesn't respect the codestyle of the file (camal case), call it 
>>> s_init_title (s for static).
>>>
>>> - I don't think the comment is needed.
>>>
>>> Best,
>>> Thomas
>>>
>>>
>>>
>>> On Tue, Jul 21, 2020, at 01:36, Daniel Glaas wrote:
>>>> Hello Thomas,
>>>>
>>>> thank you for your review and you feedback, you pointed on some 
>>>> very new
>>>> aspects for me ...
>>>>
>>>> I implemented the second alternative solution you proposed, by
>>>> introducing a special static const string with is assigned during
>>>> initialization of the variables and against which comparisons are 
>>>> done.
>>>> The way I coded this in line 75f. is surely not the best was, after
>>>> trying several minutes around, I was not able to find a better 
>>>> solution
>>>> that doesn't rise compiler warnings. I appreciate your suggestions.
>>>>
>>>> And of course you are right, the strdup("") calls lead to memory leaks
>>>> as I never freed them. I'm sorry that I didn't notice that, thanks for
>>>> noticing this.
>>>>
>>>> Am 20.07.20 um 08:33 schrieb Thomas Guillem:
>>>> > Hello,
>>>> >
>>>> > On Sat, Jul 18, 2020, at 12:47, Daniel Glaas wrote:
>>>> >> In principle, looking over all possible use cases of strdup I 
>>>> agree that
>>>> >> there might be case where strdup() returns NULL. But here I used 
>>>> it with
>>>> >> an empty string as parameter, meaning that malloc() only needs to
>>>> >> allocate one single byte in the memory. If this really fails, 
>>>> then there
>>>> >> are other problems than only a wrong display of an RSS text overlay.
>>>> > We require all allocations to be checked and handled. This is not 
>>>> the
>>>> > case for every C programs and libs, but it is in VLC.
>>>> >
>>>> >
>>>> >> I also tried to add catches to the NULL pointers at those places 
>>>> where
>>>> >> they are used. But in my opinion, this makes the places of usage 
>>>> very
>>>> >> bad to read with a lot of NULL checks at various places. A lot 
>>>> of places
>>>> >> in the *Filter() function rely on valid pointers where the 
>>>> titles are
>>>> >> stored.
>>>> >>
>>>> > I see 2 other choices :
>>>> > - fix psz_title NULLITY check everywhere
>>>> > - Assign it to a special static const string (likely "") : in that
>>>> > case, you should check that the pointer is not equals to this 
>>>> special
>>>> > string pointer before freeing it.
>>>> >
>>>> > Also, your patch seems to leak the "" allocation when psz_title is
>>>> > replaced by a new string.
>>>> >
>>>> >> Am 18.07.20 um 09:41 schrieb Rémi Denis-Courmont:
>>>> >>> Le lauantaina 18. heinäkuuta 2020, 5.07.00 EEST Daniel Glaas a 
>>>> écrit :
>>>> >>>> Initialize title strings with non NULL pointer
>>>> >>>>
>>>> >>>> If no valid title could be parsed out of an RSS feed, the
>>>> >>>> segmentation fault
>>>> >>>> caused VLC to crash. The reason therefore was that the expected
>>>> >>>> pointers to
>>>> >>>> the char buffers containing the titles were NULL pointers.
>>>> >>> This won't work because strdup() can return NULL. Better handle 
>>>> the NULL
>>>> >>> values correctly.
>>>> >>>
>>>> >> _______________________________________________
>>>> >> vlc-devel mailing list
>>>> >> To unsubscribe or modify your subscription options:
>>>> >> https://mailman.videolan.org/listinfo/vlc-devel
>>>> > _______________________________________________
>>>> > vlc-devel mailing list
>>>> > To unsubscribe or modify your subscription options:
>>>> > https://mailman.videolan.org/listinfo/vlc-devel
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>>
>>>> *Attachments:*
>>>>
>>>>   * 0003-Initialize-title-strings-with-static-const-empty-cha.patch
>>>>
>>>
>>>
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>>
>> *Attachments:*
>>
>>   * 0003-Initialize-title-strings-with-static-const-empty-cha.patch
>>
>
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200722/095376a8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Initialize-title-strings-with-static-const-empty-cha.patch
Type: text/x-patch
Size: 4257 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200722/095376a8/attachment.bin>