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
The sanity checks look reasonable, but I don't believe they're quite
enough. My original statement was a little imprecise: shifts by the
value's exact bitwidth are also undefined behavior (i.e. 1<<32 is UB).
Thus, chunksz == size == sizeof(x) is a legitimate invocation of
get_value() but requires some additional handling in addition to the
sanity checks.
Nickolai.
On Thu, Jan 3, 2013 at 4:17 AM, nick clifton <nickc@redhat.com> wrote:
> Hi Nickolai,
>
>
>> In C, shifting a value by more than its bitwidth is undefined behavior.
>> The get_value() function in bfd/elflink.c (invoked as part of
>> bfd_elf_perform_complex_relocation) sometimes shifts a bfd_vma value by
>> more than its bitwidth.
>
>
> Actually I think that there is a bigger problems here: If (8 * chunksz) is
> more than the bitwidth of the bfd_vma type then the get_value() function is
> never going to return the value that the user expects. Even if you fix the
> shifting problem.
>
> A better patch I think therefore would be something like this:
>
> diff -u -3 -p -r1.458 elflink.c
> --- bfd/elflink.c 19 Dec 2012 19:45:43 -0000 1.458
> +++ bfd/elflink.c 3 Jan 2013 09:19:34 -0000
> @@ -7919,13 +7919,19 @@ get_value (bfd_vma size,
> {
> bfd_vma x = 0;
>
> + /* Sanity checks. */
> + if (chunksz > sizeof (x)
> + || (chunksz == sizeof (x) && size > chunksz)
> + || size < chunksz
> + || size % chunksz != 0
> + || input_bfd == NULL
> + || location == NULL)
> + abort ();
> +
>
> Do you agree ?
>
> Cheers
> Nick