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

[Daniel Glaas]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200721/7bc4bc16/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/20200721/7bc4bc16/attachment.bin>