[PATCH 1/3] util: change type of struct xlat.val to uint64_t

[Jeff Mahoney]

On 3/30/16 9:48 PM, Dmitry V. Levin wrote:
> On Wed, Mar 30, 2016 at 09:19:25PM -0400, Jeff Mahoney wrote:
>> Some ioctls have flags fields that are 64-bit.  A 32-bit val means
>> these flags will never be matched or printed.

Thanks for the review.

TBH, I expected there to be some discussion on this one and I didn't
want to get too far into the weeds before having it.  I did enough work
to get the high value btrfs flags working on my x86_64 system.  I'm fine
continuing the work, but it wasn't the primary target of my time
investment. :)

> struct xlat is used for various kinds of thing all over the code,
> and this tiny change of type might have unforeseen consequences.
> [...]
>>  struct xlat {
>> -	unsigned int val;
>> +	uint64_t val;
>>  	const char *str;
>>  };
> [...]
>> -extern const char *xlookup(const struct xlat *, const unsigned int);
>> -extern const char *xlat_search(const struct xlat *, const size_t, const =
> unsigned int);
>> +extern const char *xlookup(const struct xlat *, const uint64_t);
>> +extern const char *xlat_search(const struct xlat *, const size_t, const =
> uint64_t);
> [...]
>> -extern void printxvals(const unsigned int, const char *, const struct xl=
> at *, ...);
>> +extern void printxvals(const uint64_t, const char *, const struct xlat *=
> , ...);
> [...]
>> -extern void addflags(const struct xlat *, int);
>> -extern int printflags(const struct xlat *, int, const char *);
>> -extern const char *sprintflags(const char *, const struct xlat *, int);
>> +extern void addflags(const struct xlat *, uint64_t);
>> +extern int printflags(const struct xlat *, uint64_t, const char *);
>> +extern const char *sprintflags(const char *, const struct xlat *, uint64=
> _t);
> 
> There is a potential sign extension bug in every place where a signed int
> is passed to any function "upgraded" from int/unsigned int to uint64_t.
> 
> Have you checked all these places?

No, I didn't. You're right, this could be a problem.  How about instead:

struct xlat.val is uint64_t.
Introduce *64 routines to handle those cases.
Change the signed int argument to unsigned for the existing prototypes.
Turn those routines into wrappers for the *64 versions so that no
unexpected signed extension happens.

>> @@ -132,16 +132,16 @@ xlookup(const struct xlat *xlat, const unsigned int=
>  val)
>>  static int
>>  xlat_bsearch_compare(const void *a, const void *b)
>>  {
>> -	const unsigned int val1 = (const unsigned long) a;
>> -	const unsigned int val2 = ((const struct xlat *) b)->val;
>> +	const uint64_t val1 = (const uint64_t) a;
> 
> This looks like a bug.
> 
>> +	const uint64_t val2 = ((const struct xlat *) b)->val;
>>  	return (val1 > val2) ? 1 : (val1 < val2) ? -1 : 0;
>>  }
>> =20
>>  const char *
>> -xlat_search(const struct xlat *xlat, const size_t nmemb, const unsigned =
> int val)
>> +xlat_search(const struct xlat *xlat, const size_t nmemb, const uint64_t =
> val)
>>  {
>>  	const struct xlat *e =
>> -		bsearch((const void*) (const unsigned long) val,
>> +		bsearch((const void*) val,
>>  			xlat, nmemb, sizeof(*xlat), xlat_bsearch_compare);
> 
> This also looks like a bug.  The only reason why it doesn't cause
> regressions on 32-bit systems is that the only user of xlat_search
> passes a value of type unsigned int.

Yeah, these are obviously wrong on !64bit.

-Jeff

-- 
Jeff Mahoney
SUSE Labs

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 827 bytes
Desc: OpenPGP digital signature
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20160330/c6826b7f/attachment.bin>