This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] avoid undefined behavior due to oversized shifts
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;
>