Re: [PATCH] avoid undefined behavior due to oversized shifts

[Nickolai Zeldovich <nickolai at csail dot mit dot edu>]

Looks reasonable to me.  One additional sanity check could be chunksz
!= 0, to ensure that size % chunksz does not divide by zero (and of
course, the !=0 check should go before the %).

Nickolai

On Thu, Jan 3, 2013 at 5:54 AM, nick clifton <nickc@redhat.com> wrote:
> Hi Jan, Hi Nikolai,
>
>
>> But that wouldn't eliminate the need for fixing the shifts, as the
>> checks above don't entirely exclude the chunksz == sizeof(x) case.
>
>
> Agreed.  How about this version instead ?
>
>
> @@ -7917,31 +7917,46 @@ get_value (bfd_vma size,
>            bfd *input_bfd,
>            bfd_byte *location)
>  {
> +  int shift;
>
>    bfd_vma x = 0;
>
> +  /* Sanity checks.  */
> +  if (chunksz > sizeof (x)
> +      || size < chunksz
> +      || size % chunksz != 0
> +      || input_bfd == NULL
> +      || location == NULL)
> +    abort ();
> +
> +  if (chunksz == sizeof (x))
> +    {
> +      if (size > chunksz)
> +       abort ();
> +      shift = 0;
> +    }
> +  else
> +    shift = 8 * chunksz;
>
> +
>    for (; size; size -= chunksz, location += chunksz)
>      {
>        switch (chunksz)
>         {
> -       default:
> -       case 0:
> -         abort ();
>
>         case 1:
> -         x = (x << (8 * chunksz)) | bfd_get_8 (input_bfd, location);
> +         x = (x << shift) | bfd_get_8 (input_bfd, location);
>
>           break;
>         case 2:
> -         x = (x << (8 * chunksz)) | bfd_get_16 (input_bfd, location);
> +         x = (x << shift) | bfd_get_16 (input_bfd, location);
>
>           break;
>         case 4:
> -         x = (x << (8 * chunksz)) | bfd_get_32 (input_bfd, location);
> +         x = (x << shift) | bfd_get_32 (input_bfd, location);
>           break;
> -       case 8:
>
>  #ifdef BFD64
> -         x = (x << (8 * chunksz)) | bfd_get_64 (input_bfd, location);
> -#else
> -         abort ();
> -#endif
> +       case 8:
> +         x = (x << shift) | bfd_get_64 (input_bfd, location);
>           break;
> +#endif
> +       default:
> +         abort ();
>         }
>      }
>    return x;
>