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

[Mark Wielaard <mjw at redhat dot com>]

On Fri, 2015-09-11 at 12:22 -0700, Roland McGrath wrote:
> It looks fine to me from a quick skim, but Mark should review and test it too.

I am not super enthusiastic about this change, it seems to just take
away type/size information that the compiler/bounds checking tools can
use.

That said I tested the patch under valgrind and with gcc
-fsanitize-undefined which didn't find any issues.

Some questions that might show how/why it could still be an improvement:

>     Do without union of variable length arrays.
>
>     Prepare to compile without gnu99 extension.

I think we still want to compile with several gnu99 extensions. Or is
your aim to build with -std=c99?

>     A union like
>       { T32 a32[n]; T64 a64[n]; } u;
>     is expanded to
>       void *data = malloc (...);
>       T32 *a32 = data;
>       T64 *a64 = data;

That might be useful since that prevents some possibly unbounded stack
usage. But most of the patch doesn't seem to be about that. Because such
unions are not allocated on the stack in the first place. There are a
couple of cases like the above, but there you replace them with an
alloca, which isn't much better.

> @@ -368,16 +372,15 @@ find_prelink_address_sync (Dwfl_Module *mod, struct dwfl_file *file)
>  
>    GElf_Addr undo_interp = 0;
>    {
> -    typedef union
> -    {
> -      Elf32_Phdr p32[phnum];
> -      Elf64_Phdr p64[phnum];
> -    } phdr;
> -    phdr *phdrs = malloc (sizeof (phdr));
> +    const int phdrs_bytes =
> +        phnum * MAX (sizeof (Elf32_Phdr), sizeof (Elf64_Phdr));
+    void *phdrs = malloc (phdrs_bytes);
> +    Elf32_Phdr *p32 = phdrs;
> +    Elf64_Phdr *p64 = phdrs;
>      if (unlikely (phdrs == NULL))
>        return DWFL_E_NOMEM;

phdrs_bytes should be a size_t. And it needs a check that it didn't
overflow. I am assuming such an overflow check was done by the compiler
for the previous construct. But if not, then a change like this plus an
explicit overflow check would be an improvement.

The same pattern using an const int is used a couple of times.

> --- a/src/readelf.c
> +++ b/src/readelf.c
> @@ -4943,7 +4943,7 @@ print_cfa_program (const unsigned char *readp, const unsigned char *const endp,
>                    unsigned int version, unsigned int ptr_size,
>                    Dwfl_Module *dwflmod, Ebl *ebl, Dwarf *dbg)
>  {
> -  char regnamebuf[REGNAMESZ];
> +  char *regnamebuf = alloca (REGNAMESZ);

REGNAMESZ is a constant (defined as 16), why replace with with an alloca?

> @@ -8379,11 +8379,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 = alloca (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

count is actually bounded, but that might not be easy to see.
We trust the backends to only provide sane Ebl_Core_Item's. The maximum
number of items I see is in the tilegx backends (which has an item with
count = 56). So this is fine, but maybe a we should add a comment
explaining it (or maybe add an assert (count < 128) or something to make
sure if this every accidentally gets violated we get an warning/error
early (and not silent stack corruption).

> @@ -8900,8 +8905,9 @@ handle_core_registers (Ebl *ebl, Elf *core, const void *desc,
>        assert (maxnreg > 0);
>      }
>  
> -  struct register_info regs[maxnreg];
> -  memset (regs, 0, sizeof regs);
> +  const int sizeof_regs = sizeof (struct register_info) * maxnreg;
> +  struct register_info *regs = alloca (sizeof_regs);
> +  memset (regs, 0, sizeof_regs);
>  
>    /* Sort to collect the sets together.  */
>    int maxreg = 0;

Same here. We count on ebl_register_info to return a (small) bounded number.

> @@ -1013,13 +1017,13 @@ find_alloc_sections_prelink (Elf *debug, Elf_Data *debug_shstrtab,
>         error (EXIT_FAILURE, 0, _("invalid contents in '%s' section"),
>                ".gnu.prelink_undo");
>  
> -      union
> -      {
> -       Elf32_Shdr s32[shnum - 1];
> -       Elf64_Shdr s64[shnum - 1];
> -      } shdr;
> -      dst.d_buf = &shdr;
> -      dst.d_size = sizeof shdr;
> +      const int shdr_bytes =
> +          (shnum - 1) * MAX (sizeof (Elf32_Shdr), sizeof (Elf64_Shdr));
> +      void *shdr = alloca (shdr_bytes);
> +      Elf32_Shdr *s32 = shdr;
> +      Elf64_Shdr *s64 = shdr;
> +      dst.d_buf = shdr;
> +      dst.d_size = shdr_bytes;

Here, and in copy_elided_sections there are real stack allocated
(possibly unbounded) variable length arrays. I think it makes sense to
actually use malloc/free instead of alloca. Otherwise you just keep the
problem. The reason this wasn't caught before is because this file isn't
build with -Wstack-usage. You don't have to make them -Wstack-usage
clean, but if you are replacing real VLAs anyway, then please don't
replace them with alloca if at all possible.

Thanks,

Mark