[vlc-devel] [PATCH 3/3] Initialize title strings with non NULL pointer
daniel.glaas at freenet.de
Tue Jul 21 21:47:19 CEST 2020
Please find attached my reworked commit, I think I implemented all your
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.
> 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
>> >> 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
>> >> 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:
>> * 0003-Initialize-title-strings-with-static-const-empty-cha.patch
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
-------------- next part --------------
An HTML attachment was scrubbed...
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 4257 bytes
Desc: not available
More information about the vlc-devel