Re: [Xen-devel] [PATCH v5 14/16] x86/boot: implement early command line parser in C

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.08.16 at 21:58, <daniel.kiper@xxxxxxxxxx> wrote:
> On Thu, Aug 25, 2016 at 07:27:21AM -0600, Jan Beulich wrote:
>> >>> On 20.08.16 at 00:43, <daniel.kiper@xxxxxxxxxx> wrote:
>> > +#define NULL      ((void *)0)
>> > +
>> > +#define __packed  __attribute__((__packed__))
>> > +#define __stdcall __attribute__((__stdcall__))
>> > +
>> > +#define max(x,y) ({ \
>> > +        const typeof(x) _x = (x);       \
>> > +        const typeof(y) _y = (y);       \
>> > +        (void) (&_x == &_y);            \
>> > +        _x > _y ? _x : _y; })
>>
>> Now that you add a second instance of (some of) these, please
>> move them to a new (local to this directory) header, e.g. defs.h.
>>
>> > +#define tolower(c) ((c) | 0x20)
>> > +
>> > +typedef unsigned char bool_t;
>>
>> _Bool and bool please and ...
>>
>> > +typedef unsigned char u8;
>> > +typedef unsigned short u16;
>> > +typedef unsigned int size_t;
>> > +
>> > +#define FALSE             0
>> > +#define TRUE              1
>>
>> ... these replaced by true and false. In fact I see no reason why
>> you couldn't include xen/stdbool.h here now that it can be more
>> generally used.
> 
> Maybe we should include xen/stdbool.h in defs.h and move to it (defs.h)
> all macros, types definitions, etc. from reloc.c and cmdline.c. This way
> we can share more stuff without duplicating it in both files. Does it
> make sense?

Well, that's what I did suggest in the very first comment still visible
in context above.

>> > +/*
>> > + * static const char *delim_chars = &delim_chars_comma[1];
>> > + *
>> > + * Older compilers, e.g. gcc version 4.1.2 20061115 (prerelease) (Debian 
>> > 4.1.1-21),
>> > + * put &delim_chars_comma[1] directly into *delim_chars. This means that 
>> > the address
>> > + * in *delim_chars is not properly updated during runtime. Newer 
>> > compilers are much
>> > + * smarter and build fully relocatable code even if above shown construct 
>> > is used.
>> > + * However, define delim_chars[] separately to properly build Xen code on
>> > + * older systems.
>> > + */
>>
>> I have to admit that I don't really understand what you want to
>> say with this comment.
> 
> I tried to use following thing suggested by you earlier:
>   static const char *delim_chars = &delim_chars_comma[1];

I've never suggested anything like that. All I suggested is the
#define approach you've now said you'd give a try.

>> > +static unsigned int strtoui(const char *s, const char *stop, const char 
>> > **next)
>> > +{
>> > +    char l;
>> > +    unsigned int base = 10, ores = 0, res = 0;
>> > +
>> > +    if ( *s == '0' )
>> > +      base = (tolower(*++s) == 'x') ? (++s, 16) : 8;
>> > +
>> > +    for ( ; *s != '\0'; ++s )
>> > +    {
>> > +        if ( stop && strchr(stop, *s) )
>> > +            goto out;
>> > +
>> > +        if ( *s < '0' || (*s > '7' && base == 8) )
>> > +        {
>> > +            res = UINT_MAX;
>> > +            goto out;
>> > +        }
>> > +
>> > +        l = tolower(*s);
>> > +
>> > +        if ( *s > '9' && (base != 16 || l < 'a' || l > 'f') )
>> > +        {
>> > +            res = UINT_MAX;
>> > +            goto out;
>> > +        }
>> > +
>> > +        res *= base;
>> > +        res += (l >= 'a') ? (l - 'a' + 10) : (*s - '0');
>> > +
>> > +        if ( ores > res )
>> > +        {
>> > +            res = UINT_MAX;
>> > +            goto out;
>> > +        }
>>
>> Without having spent time to try and find an example, it feels like this
>> check won't catch all possible overflow conditions. If you care about
>> overflow, please make sure you catch all cases.
> 
> Hmmm.... How come? Could you give an example?

Excuse me, but shouldn't you instead demonstrate the logic is
correct? Or - consider what I had said - try to find an example
yourself? It's not that difficult: With 16-bit word size
0x3333 * 10 = 0x1fffe, which truncates to 0xfffe and is hence
larger than both inputs but still produced an overflow. This
easily extends to 32- and 64-bit word size.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel