[vlc-devel] [PATCH] access/satip: remove write-only variable

[Filip Roséen]

Hi Rémi,

Sorry for taking a while to reply to your message; I must have
accidentally marked it as read during my most recent flight, and only
now discovered that I never took any action regarding it.

------------------------------------------------------------------------

On 2017-03-01 16:14, Rémi Denis-Courmont wrote:

> On March 1, 2017 10:52:09 AM GMT+02:00, "Filip Roséen" <filip at atch.se> wrote:
>
> >--- a/modules/access/satip.c
> >+++ b/modules/access/satip.c
> >@@ -645,10 +645,8 @@ static int satip_open(vlc_object_t *obj)
> >     for (unsigned i = 0; i < strlen(psz_lower_url); i++)
> >         psz_lower_url[i] = tolower(psz_lower_url[i]);
> > 
> >-    const char* psz_lower_location = strstr( psz_lower_url, "://" );
> >-    if ( psz_lower_location == NULL )
> >+    if( strstr( psz_lower_url, "://" ) == NULL )
> >         goto error;
> >-    psz_lower_location += 3;
> 
> Valid point but isn't the strstr() too redundant with UrlParse?

Regarding the usage of `strstr` I was reluctant to remove it as I
feared that it would be a change of behavior, but as `access->psz_url`
is guaranteed to contain the searched for substring (because of the
check in `src/input/access.c:access_New` which also looks for `://`)
it is indeed redundant.

Given that the *return-value* of `vlc_UrlParse` is not checked, nor
the actual state of the resulting `struct` is properly checked for
errors, the redundancy should not be because of usage of the function.

Attach to this email is a patch that removes the useless code, but I
will follow up with more fixes for the module in question as soon as
possible (see next section).

Best Regards,\
Filip

------------------------------------------------------------------------

Further thoughts
------------------------------------------------------------------------

As it turns out, `satip.c:Open` needs to be fixed far deeper than
removing a write-only variable. Your messaged caused me to look at the
implementation in more detail, and as it turns out there are numerous
ways in which the code will invoke *undefined-behavior* due to
erroneous assumptions.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170302/d1c10388/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-access-satip-remove-write-only-variable.patch
Type: text/x-diff
Size: 1499 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170302/d1c10388/attachment.patch>