Re: [PATCH] Do without union of variable length arrays.

[Mark Wielaard <mjw at redhat dot com>]

Hi,

On Wed, 2015-09-16 at 16:16 -0700, Chih-hung Hsieh wrote:
> * I used alloca to keep the same functionality,
>   but now they are replaced with malloc or xmalloc.

Thanks, that looks good. But not all needed to change to malloc, when we
know the size is bounded it is simpler to use alloca I think. See below.

> * Now const size_t is used instead of const int for malloc argument type.

Thanks. I am still interested in the overflow issue. I believe since we
are using unsigned arithmetic and we know the size is always > 0, it
should be as simple as doing:

  const size_t elem_size = ... sizeof (...);
  const size_t bytes = num * elem_size;
  if (unlikely (bytes / elem_size != num))
    return E_NOMEM;
  ... malloc (bytes);

> * I added assert(count<128) with simple comment,
>   but I am not familiar with all the size assumptions to add extra checks.
>   Could someone more familiar with these modules do it?

Yes, sorry. I think the assert you added is all that is needed. I'll
suggest better wording for the comment. It was mainly to help review the
changes. I wanted to make sure we weren't accidentally doing unbounded
(stack) allocations.

> It's unfortunate that we will lose some compile time bound checking
> from gnu-compatible tools. I think all clang based tools cannot
> handle such VLA in union or struct anyway. I hope this is a reasonable
> trade off to get wider use of elfutils on Android, which is moving fast
> to use clang as the default compiler.

I appreciate your end goal, but lets take it one patch at a time. Your
patches and the reviews have improved the code which is a good thing.
Thanks for taking the time to address the issues. It has been a great
help. And you pointing out some missing compiler warnings tricked me
into adding them to GCC, so that is another nice thing (really, gcc is
pretty good and easy to hack on, I am not sure using clang has any real
benefit, at least for elfutils).

elfutils is best used with the GNU toolchain though. It would be good to
support any ELF/DWARF based system if at all possible. On android you
will also have to cope with not having glibc around. elfutils has been
made to work on kfreebsd and with uClibc, so it should hopefully also
work on android with some additional work.

> @@ -8370,6 +8370,8 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
>  		  unsigned int colno, size_t *repeated_size)
>  {
>    uint_fast16_t count = item->count ?: 1;
> +  /* count is variable but should be bound. */
> +  assert (count < 128);

Assert is good. I would make the comment:
/* Ebl_Core_Item count is always a small number.
   Make sure the backend didn't put in some large bogus value.  */

>  #define TYPES								      \
>    DO_TYPE (BYTE, Byte, "0x%.2" PRIx8, "%" PRId8);			      \
> @@ -8379,11 +8381,16 @@ handle_core_item (Elf *core, const Ebl_Core_Item *item, const void *desc,
>    DO_TYPE (XWORD, Xword, "0x%.16" PRIx64, "%" PRId64);			      \
>    DO_TYPE (SXWORD, Sxword, "%" PRId64, "%" PRId64)
>  
> -#define DO_TYPE(NAME, Name, hex, dec) GElf_##Name Name[count]
> -  union { TYPES; } value;
> +#define DO_TYPE(NAME, Name, hex, dec) GElf_##Name Name
> +  typedef union { TYPES; } value_t;
> +  void *data = malloc (count * sizeof (value_t));
> +#undef DO_TYPE
> +
> +#define DO_TYPE(NAME, Name, hex, dec) \
> +    GElf_##Name *value_##Name __attribute__((unused)) = data
> +  TYPES;
>  #undef DO_TYPE
>  
> -  void *data = &value;

I think this can just stay as an alloca since we know it is a small
bounded number. But if you want to keep it as malloc you'll need to
check malloc didn't fail.

Thanks,

Mark