Warning implies possible kerberos bug in krb5int_dns_nextans() in dnsglue.c

[Jeffrey Hutzelman]

--On Monday, March 17, 2008 11:45:58 PM -0600 Philip Guenther 
<guenther at gmail.com> wrote:

> On Mon, Mar 17, 2008 at 6:36 PM, Jeffrey Hutzelman <jhutz at cmu.edu> wrote:
> ...
>>  I think both concerns are valid.  Harmless warnings should be
>>  eliminated, as possible, because doing so makes it easier to find real
>>  problem.  In this case, I would suggest replacing the offending line
>>  with the following:
>>
>>         /* NB: sizeof(unsigned short) <= sizeof(unsigned int) */
>>         if ((unsigned int)rdlen > INT_MAX)
>>
>>  This eliminates the harmless warning, but should still generate one if
>>  rdlen ever gets retyped to something bigger than an unsigned int.
>
> As Russ observed, the test is there to protect systems where short and
> int are both 16bit types.  If an EDNS0 extension let the field in the
> packet grow to 32bits and rdlen was changed to a unsigned long as a
> result, then your 'fixed' code would make the test return false
> negatives when rdlen was > UINT_MAX but less than UINT_MAX+INT_MAX
> (and similarly in higher ranges) and it would *not* generate a warning
> when compiled.

Hrm.  Indeed, an explicit cast to a smaller type does not seem to generate 
a warning, in at least one compiler I tried.  I believe it would be 
possible to work something out by taking advantage of implicit promotion of 
function call arguments, or even just eliminating the constant-ness of the 
second argument.  For example:

int too_big(unsigned int arg) { return arg > INT_MAX; }
int greater(unsigned short x, unsigned int y) { return x > y; }

However, I'm not convinced it's worth the ugliness.

> IMHO, the Right Thing would be to fix the API and make lenp point to
> an unsigned int instead of a signed int.  If I'm reading the callers
> correctly, matching them match would be trivial.

I haven't examined the surrounding code.  If the API in question is 
entirely internal, this may be a possibility.


> Alternatively, just change the code to test against SHORT_MAX.  Yeah,
> that limits it to 32kB RDATA fields, but the current code already
> imposes that limitation on systems where int is 16bits.

True, but since int is not 32 bits on most systems today, do we really want 
to impose that limitation?

-- Jeff