Please review the new attached 0004*patch file. Compared with previous 0003-Do-without-union-of-variable-length-arrays.patch the new 0004-* patch has:
* Synced in latest elfutils.
* Updated changes to ChangeLog files.
* Checked overflow before malloc or xmalloc:
xmalloc(shdr_bytes) in src/unstrip.c
malloc(phnum * phent) in libdwfl/link_map.c
malloc(phdrs_bytes) in libdwfl/dwfl_module_getdwarf.c
malloc(phdrsp_bytes) in libdwfl/dwfl_segment_report_module.c
* Used new suggested comment about assert (count < 128) in src/readelf.c.
* Reverted back to alloca for the data pointer to value_t in src/readelf.c
Thanks.
On Thu, Sep 17, 2015 at 2:40 AM, Mark Wielaard <m...@redhat.com> wrote:
> 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
>
0004-Do-without-union-of-variable-length-arrays.patch
Description: Binary data
