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

[Daniel Glaas]

Lol, I forgot to commit the adaptions to my patch before creating the 
last file...

Here it is, hopefully now in the correct version.

Am 23.07.20 um 09:39 schrieb 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
>>
>
>
> _______________________________________________
> 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/20200723/2c7931ed/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: 4229 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200723/2c7931ed/attachment.bin>