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

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