<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Thomas,</p>
<p>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.</p>
<p>I attached the corrected version of the patch.<br>
</p>
<div class="moz-cite-prefix">Am 22.07.20 um 08:45 schrieb Thomas
Guillem:<br>
</div>
<blockquote type="cite"
cite="mid:60492160-feea-4b7d-a867-95d61efe8c00@www.fastmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<title></title>
<style type="text/css">
p.MsoNormal,p.MsoNoSpacing{margin:0}</style>
<div>Hello,<br>
</div>
<div><br>
</div>
<div>Yes the cast is correct but &s_init_title[0] ==
s_init_title, so you don't need &[0].<br>
</div>
<div><br>
</div>
<div>On Tue, Jul 21, 2020, at 21:47, Daniel Glaas wrote:<br>
</div>
<blockquote type="cite" id="qt" style="">
<p>Please find attached my reworked commit, I think I
implemented all your remarks.<br>
</p>
<p>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.<br>
</p>
<div class="qt-moz-cite-prefix">Am 21.07.20 um 11:20 schrieb
Thomas Guillem:<br>
</div>
<blockquote type="cite"
cite="mid:90e9f0a3-93a9-44e9-8b0a-db82b8252e35@www.fastmail.com">
<div>Your commit looks good. Few remarks:<br>
</div>
<div><br>
</div>
<div>- You don't need p_initTitle, you can use initTitle
directly<br>
</div>
<div><br>
</div>
<div>- 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).<br>
</div>
<div><br>
</div>
<div>- I don't think the comment is needed.<br>
</div>
<div><br>
</div>
<div>Best,<br>
</div>
<div>Thomas<br>
</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div>On Tue, Jul 21, 2020, at 01:36, Daniel Glaas wrote:<br>
</div>
<blockquote type="cite" id="qt-qt" style="">
<div>Hello Thomas,<br>
</div>
<div><br>
</div>
<div>thank you for your review and you feedback, you pointed
on some very new <br>
</div>
<div>aspects for me ...<br>
</div>
<div><br>
</div>
<div>I implemented the second alternative solution you
proposed, by <br>
</div>
<div>introducing a special static const string with is
assigned during <br>
</div>
<div>initialization of the variables and against which
comparisons are done. <br>
</div>
<div>The way I coded this in line 75f. is surely not the
best was, after <br>
</div>
<div>trying several minutes around, I was not able to find a
better solution <br>
</div>
<div>that doesn't rise compiler warnings. I appreciate your
suggestions.<br>
</div>
<div><br>
</div>
<div>And of course you are right, the strdup("") calls lead
to memory leaks <br>
</div>
<div>as I never freed them. I'm sorry that I didn't notice
that, thanks for <br>
</div>
<div>noticing this.<br>
</div>
<div><br>
</div>
<div>Am 20.07.20 um 08:33 schrieb Thomas Guillem:<br>
</div>
<div>> Hello,<br>
</div>
<div>><br>
</div>
<div>> On Sat, Jul 18, 2020, at 12:47, Daniel Glaas
wrote:<br>
</div>
<div>>> In principle, looking over all possible use
cases of strdup I agree that<br>
</div>
<div>>> there might be case where strdup() returns
NULL. But here I used it with<br>
</div>
<div>>> an empty string as parameter, meaning that
malloc() only needs to<br>
</div>
<div>>> allocate one single byte in the memory. If
this really fails, then there<br>
</div>
<div>>> are other problems than only a wrong display
of an RSS text overlay.<br>
</div>
<div>> We require all allocations to be checked and
handled. This is not the <br>
</div>
<div>> case for every C programs and libs, but it is in
VLC.<br>
</div>
<div>><br>
</div>
<div>><br>
</div>
<div>>> I also tried to add catches to the NULL
pointers at those places where<br>
</div>
<div>>> they are used. But in my opinion, this makes
the places of usage very<br>
</div>
<div>>> bad to read with a lot of NULL checks at
various places. A lot of places<br>
</div>
<div>>> in the *Filter() function rely on valid
pointers where the titles are<br>
</div>
<div>>> stored.<br>
</div>
<div>>><br>
</div>
<div>> I see 2 other choices :<br>
</div>
<div>> - fix psz_title NULLITY check everywhere<br>
</div>
<div>> - Assign it to a special static const string
(likely "") : in that <br>
</div>
<div>> case, you should check that the pointer is not
equals to this special <br>
</div>
<div>> string pointer before freeing it.<br>
</div>
<div>><br>
</div>
<div>> Also, your patch seems to leak the "" allocation
when psz_title is <br>
</div>
<div>> replaced by a new string.<br>
</div>
<div>><br>
</div>
<div>>> Am 18.07.20 um 09:41 schrieb Rémi
Denis-Courmont:<br>
</div>
<div>>>> Le lauantaina 18. heinäkuuta 2020, 5.07.00
EEST Daniel Glaas a écrit :<br>
</div>
<div>>>>> Initialize title strings with non NULL
pointer<br>
</div>
<div>>>>><br>
</div>
<div>>>>> If no valid title could be parsed out
of an RSS feed, the <br>
</div>
<div>>>>> segmentation fault<br>
</div>
<div>>>>> caused VLC to crash. The reason
therefore was that the expected <br>
</div>
<div>>>>> pointers to<br>
</div>
<div>>>>> the char buffers containing the titles
were NULL pointers.<br>
</div>
<div>>>> This won't work because strdup() can
return NULL. Better handle the NULL<br>
</div>
<div>>>> values correctly.<br>
</div>
<div>>>><br>
</div>
<div>>>
_______________________________________________<br>
</div>
<div>>> vlc-devel mailing list<br>
</div>
<div>>> To unsubscribe or modify your subscription
options:<br>
</div>
<div>>> <a
href="https://mailman.videolan.org/listinfo/vlc-devel"
moz-do-not-send="true">https://mailman.videolan.org/listinfo/vlc-devel</a><br>
</div>
<div>> _______________________________________________<br>
</div>
<div>> vlc-devel mailing list<br>
</div>
<div>> To unsubscribe or modify your subscription
options:<br>
</div>
<div>> <a
href="https://mailman.videolan.org/listinfo/vlc-devel"
moz-do-not-send="true">https://mailman.videolan.org/listinfo/vlc-devel</a><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div><br>
</div>
<div>_______________________________________________<br>
</div>
<div>vlc-devel mailing list<br>
</div>
<div>To unsubscribe or modify your subscription options:<br>
</div>
<div><a
href="https://mailman.videolan.org/listinfo/vlc-devel"
moz-do-not-send="true">https://mailman.videolan.org/listinfo/vlc-devel</a><br>
</div>
<div><br>
</div>
<div><b>Attachments:</b><br>
</div>
<ul>
<li>0003-Initialize-title-strings-with-static-const-empty-cha.patch<br>
</li>
</ul>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<pre class="qt-moz-quote-pre">_______________________________________________
vlc-devel mailing list
To unsubscribe or modify your subscription options:
<a class="qt-moz-txt-link-freetext" href="https://mailman.videolan.org/listinfo/vlc-devel" moz-do-not-send="true">https://mailman.videolan.org/listinfo/vlc-devel</a>
</pre>
</blockquote>
<div>_______________________________________________<br>
</div>
<div>vlc-devel mailing list<br>
</div>
<div>To unsubscribe or modify your subscription options:<br>
</div>
<div><a href="https://mailman.videolan.org/listinfo/vlc-devel"
moz-do-not-send="true">https://mailman.videolan.org/listinfo/vlc-devel</a><br>
</div>
<div><br>
</div>
<div><b>Attachments:</b><br>
</div>
<ul>
<li>0003-Initialize-title-strings-with-static-const-empty-cha.patch<br>
</li>
</ul>
</blockquote>
<div><br>
</div>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<pre class="moz-quote-pre" wrap="">_______________________________________________
vlc-devel mailing list
To unsubscribe or modify your subscription options:
<a class="moz-txt-link-freetext" href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a></pre>
</blockquote>
</body>
</html>