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

[Thomas Guillem]

You are welcome, and thanks for your contribution!

I suggest you the following:

- p_feed->p_items[p_feed->i_items-1].psz_title = (char *)&s_init_title[0]; 
+ p_feed->p_items[p_feed->i_items-1].psz_title = (char *)s_init_title;

Same for the other part of the code.

Best,
Thomas

On Wed, Jul 22, 2020, at 22:49, Daniel Glaas wrote:
> 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
> _______________________________________________
> 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/20200723/a8511b8b/attachment.html>