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

[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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200722/b18282e7/attachment.html>