[vlc-devel] [PATCH 4/4] text/url: fix port handling in vlc_UrlParse

[Filip Roséen]

Hi Rémi,

On 2016-10-28 15:24, Rémi Denis-Courmont wrote:

> > +            if (strchr("0123456789", *next) == NULL || *end ||
> 
> (((unsigned char)*next) - '0' < 10) is probably much faster/smaller.

But also less readable, especially if written correctly. I do not
imagine that the potential overhead of calling `strchr` that we have
to worry about.

One could write an expression that will be faster (in theory), but I'd
view that as micro-optimization (with little gain given the
readability issue it would introduce).

> 
> > +                port > UINT_MAX || (port == ULONG_MAX && errno == ERANGE))
> 
> I think that this will cause warnings on 32-bits architectures because
> port > UINT_MAX is impossible.

Given the semantics of `UINT_MAX` and `ULONG_MAX`, I would assume that
any modern compiler would not warn on such usage, and just to verify I
created the below test-case.

    /tmp% cat bar.c
    #include <limits.h>
    #include <assert.h>
    
    int main()
    {
        static_assert( ULONG_MAX == UINT_MAX, "!!" );
    
        unsigned long x = ULONG_MAX;
    
        if( x > UINT_MAX )
            return 0;
    
        return 1;
    }
    /tmp% gcc -Wall -Wextra -pedantic -std=c11 -m32 -c bar.c
    /tmp% clang -Wall -Wextra -pedantic -std=c11 -m32 -c bar.c 
    /tmp%

I also tested with all the 32bit compilers on *godbolt.org*, with a modified
testcase since not all of them has support for C11, and I did not see any such
warning.

Though if it turns into an issue I would be happy to prevent such warning.

> And you cannot rely on errno because it could be the result of an earlier 
> function call failing.

I have attached an update patch where the state of `errno` is set to `0` prior
to the invocation of `strtoul`, and reset back to whatever state it had before
(unless we find invalid data) after.

Thanks for the input!

Best Regards,\
Filip

> > +            {
> > +                errno = EINVAL;
> >                  ret = -1;
> > -#endif
> > +            }
> > +
> > +            url->i_port = port;
> >          }
> > 
> >          if (url->psz_path != NULL)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161028/4d86c2f6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-text-url-fix-port-handling-in-vlc_UrlParse.patch
Type: text/x-diff
Size: 1810 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161028/4d86c2f6/attachment.patch>