This is the mail archive of the elfutils-devel@sourceware.org mailing list for the elfutils project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Mark,

I have made the changes you suggested.
Please take a look of attached 0003*.patch again.

* The comment is changed.  I don't plan to change -std=gnu99 mode yet.
  These changes only avoid some gnu extensions that clang does not like.

* I used alloca to keep the same functionality,
  but now they are replaced with malloc or xmalloc.

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

* 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?

* I reverted the changes of on-stack variable length arrays.
  They can be accepted by clang, but not captured into clang Blocks closure.
  Assuming that you prefer replacing nested functions with macros
  or file scope functions, those on-stack VLA can be kept as is:

    char regnamebuf[REGNAMESZ];

    struct register_info regs[maxnreg];
    memset (regs, 0, sizeof regs);

    struct section sections[stripped_shnum - 1];


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.

Thanks.


On Wed, Sep 16, 2015 at 8:25 AM, Mark Wielaard <mjw@redhat.com> wrote:

> 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
>
Mark,

I have made the changes you suggested.
Please take a look of attached 0003*.patch again.

* The comment is changed.  I don't plan to change -std=gnu99 mode yet.
  These changes only avoid some gnu extensions that clang does not like.

* I used alloca to keep the same functionality,
  but now they are replaced with malloc or xmalloc.

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

* 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?

* I reverted the changes of on-stack variable length arrays.
  They can be accepted by clang, but not captured into clang Blocks closure.
  Assuming that you prefer replacing nested functions with macros
  or file scope functions, those on-stack VLA can be kept as is:
  
    char regnamebuf[REGNAMESZ];

    struct register_info regs[maxnreg];
    memset (regs, 0, sizeof regs);

    struct section sections[stripped_shnum - 1];


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.

Thanks.


On Wed, Sep 16, 2015 at 8:25 AM, Mark Wielaard <mjw@redhat.com> wrote:
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 = "" (...);
>       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 = "" (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

Attachment: 0003-Do-without-union-of-variable-length-arrays.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]