C++ Logo

sg12

Advanced search

Re: [ub] ub due to left operand of shift

From: Gabriel Dos Reis <gdr_at_[hidden]>
Date: Fri, 25 Oct 2013 14:50:26 -0500
John Regehr <regehr_at_[hidden]> writes:

| > What reason do you have to believe that crypto is using any signed
| > arithmetic? I would not.
|
| Here's an example that's at least slightly interesting, from the latest
| version of LibTomCrypt:
|
| kappa[i] =
| (key[pos ] << 24) ^
| (key[pos + 1] << 16) ^
| (key[pos + 2] << 8) ^
| (key[pos + 3] );
|
| key is a pointer to unsigned char. Of course, the array element becomes
| signed after promotion. The shift by 24 then executes an undefined
| behavior whenever the shifted value is >127.
|
| So the interesting thing is that the developer is basically doing things
| right and getting hosed by the arithmetic conversions. I'm not sure
| there's a fix for this that's prettier than an explicit cast to
| unsigned? That's unfortunate since it breaks the symmetry of the four
| cases; of course the other three do not require the cast.

If the developer is using key as an array of unsigned char, he
cannot possibly be "doing things right", for as you correctly pointed
out there is an arithmetic promotion -- since the K&R days. Even when
we would like to see changes, it still remains our job to be careful in
assigning "right" or "correct".

I would have exepected

      inline uint32_t lshift_by(uint32_t n, int b = 0) {
          return n << b;
      }

(replace 'inline' with 'constexpr' if you prefer.)

  kappa[i] = lshift_by(key[pos ], 24) ^
             lshift_by(key[pos + 1], 16) ^
             lshift_by(key[pos + 2], 8) ^
             lshift_by(key[pos + 3] );

-- Gaby

Received on 2013-10-25 21:50:41