On Mon, 2015-10-26 at 23:00 +0300, Alexander Cherepanov wrote:
> On 2015-10-07 18:23, Mark Wielaard wrote:
> >>> You can build and run elfutils
> >>> and the tests with configure --enable-sanitize-undefined to use ubsan
> >>> checking.
> >>
> >> Nice.
> >
> > I am using it together with the afl fuz
On 2015-10-07 18:23, Mark Wielaard wrote:
On Wed, 2015-10-07 at 02:24 +0300, Alexander Cherepanov wrote:
On 2015-10-05 21:45, Mark Wielaard wrote:
On Fri, Oct 02, 2015 at 12:10:47AM +0300, Alexander Cherepanov wrote:
Given that the current approach (before the patch) already required to write
On Wed, 2015-10-07 at 16:46 -0700, Chih-hung Hsieh wrote:
> Your 0001-Allocate*path looked very clean to me.
Thanks, pushed to master.
Mark,
Your 0001-Allocate*path looked very clean to me.
I look forward to this change for merging into Android.
After this, the only thing clang complains about should be the nested
functions.
Yes, it is good to use new tools for additional checks, as long as there is
still a way to compile with o
On Wed, 2015-10-07 at 09:15 -0700, Chih-hung Hsieh wrote:
> > I am currently using gcc 5.1 where it definitely works.
> > -fsanitize=undefined finds array accesses outside variable arrays just
> > fine. It didn't work with gcc 4.8 though. I thought it also worked with
> > 4.9, but haven't checked.
Looks sensible to me.
On Wed, Oct 07, 2015 at 02:38:45AM +0300, Alexander Cherepanov wrote:
> If the amount of allocated memory is explicitly calculated as above it's
> potentially possible to use exact amount, e.g. phnum * sizeof (Elf32_Phdr)
> for p32 (and assign NULL to p64) or phnum * MAX (sizeof (Elf64_Phdr) for p6
On Tue, Oct 06, 2015 at 04:17:16PM -0700, Chih-hung Hsieh wrote:
> Please take a look of the new attached 0007*patch.
Thanks, pushed to master. I thought Alexander had a good point,
but we can deal with that in a later patch.
Cheers,
Mark
>
> > >>void *data = malloc (...);
> > >>T32 (*a32)[n] = data;
> > >>T64 (*a64)[n] = data;
> > >>
> > >> Then the use looks like "(*a32)[i].member". Clang seems to be happy
> and its
> > >> UBSAN works fine.
> > >
> > > If that works that would probably be preferred since then ubsan can
On Wed, 2015-10-07 at 02:24 +0300, Alexander Cherepanov wrote:
> On 2015-10-05 21:45, Mark Wielaard wrote:
> > On Fri, Oct 02, 2015 at 12:10:47AM +0300, Alexander Cherepanov wrote:
> >> Given that the current approach (before the patch) already required to
> >> write
> >> superfluous "->" perhaps
On 2015-10-07 02:17, Chih-hung Hsieh wrote:
> +if (unlikely (phnum >
> + SIZE_MAX / MAX (sizeof (Elf32_Phdr), sizeof
(Elf64_Phdr
> + return DWFL_E_NOMEM;
> +const size_t phdrs_bytes =
> +phnum * MAX (sizeof (Elf32_Phdr), sizeof (Elf64_Phdr));
> +void
On 2015-10-05 21:45, Mark Wielaard wrote:
On Fri, Oct 02, 2015 at 12:10:47AM +0300, Alexander Cherepanov wrote:
On 2015-09-16 18:25, Mark Wielaard 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
Mark,
I appreciate your efforts to make these and other changes about nested
functions. We can see soon elfutils will compile with clang/llvm on Android.
Please take a look of the new attached 0007*patch.
Thanks.
On Tue, Oct 6, 2015 at 2:25 PM, Mark Wielaard wrote:
> On Mon, 2015-10-05 at 14:1
On Mon, 2015-10-05 at 14:15 -0700, Chih-hung Hsieh wrote:
> Please review attached 0006*patch with the new changed I mentioned
> previously. Thanks.
Very nice. Now we get to keep the array bounds.
You forgot them in one place though:
> diff --git a/libelf/elf_getarsym.c b/libelf/elf_getarsym.c
>
Please review attached 0006*patch with the new changed I mentioned
previously. Thanks.
On Mon, Oct 5, 2015 at 12:04 PM, Chih-hung Hsieh wrote:
> I will prepare a new patch with the suggested changes:
>
> (a) more efficient overflow check like
> if (num > SIZE_MAX / elem_size)
>
> (b) r
I will prepare a new patch with the suggested changes:
(a) more efficient overflow check like
if (num > SIZE_MAX / elem_size)
(b) replacing union of VLA with pointers to arrays like
void *data = malloc (...);
T32 (*a32)[n] = data;
T64 (*a64)[n] = data;
Please f
On Fri, Oct 02, 2015 at 04:17:43PM +0200, Florian Weimer wrote:
> On 09/08/2015 11:08 PM, Chih-Hung Hsieh wrote:
> > +void *phdrs = malloc (phnum * sizeof (phdr_u));
>
> If you change this code anyway, it's sensible to check for integer
> overflow in the size computation.
This is now done in
On Fri, Oct 02, 2015 at 12:10:47AM +0300, Alexander Cherepanov wrote:
> On 2015-09-16 18:25, Mark Wielaard 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 ab
On 01.10.2015 23:45, Alexander Cherepanov wrote:
>On 2015-09-17 12:40, Mark Wielaard wrote:
>>>* 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 si
[Fixed typo and restored lost Cc, sorry.]
On 01.10.2015 23:45, Alexander Cherepanov wrote:
On 2015-09-17 12:40, Mark Wielaard wrote:
* 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 unsi
On 09/08/2015 11:08 PM, Chih-Hung Hsieh wrote:
> +void *phdrs = malloc (phnum * sizeof (phdr_u));
If you change this code anyway, it's sensible to check for integer
overflow in the size computation.
Thanks,
Florian
On Thu, Oct 1, 2015 at 2:10 PM, Alexander Cherepanov
wrote:
> On 2015-09-16 18:25, Mark Wielaard 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 abo
On 2015-09-16 18:25, Mark Wielaard 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/bo
On 2015-09-17 12:40, Mark Wielaard wrote:
* 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:
cons
Thanks. I updated ChangLog files in the new attached 0005*patch file.
I think we mean the new assert(count<128) statement in readelf.c.
It is now explained in ChangeLog that assert was added because alloca was
used for the new dynamic allocation. For other malloc and xmalloc, overflow
tests were a
In ChangeLog entries, "Likewise" should be followed by a period.
You have some readelf changes that are not directly part of avoiding
unions. They are also not described in the src/ChangeLog entry.
Please post those separately and describe their justifications.
Thanks,
Roland
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/un
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 thi
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
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 te
It looks fine to me from a quick skim, but Mark should review and test it too.
The attached new 0002*patch file has ChangeLog files and uses MAX to
replace union types. Please take a look again.
Thanks.
On Tue, Sep 8, 2015 at 2:15 PM, Roland McGrath wrote:
> That looks generally OK, though it lacks ChangeLog entries.
> But I think it's a bit weird to declare those union t
That looks generally OK, though it lacks ChangeLog entries.
But I think it's a bit weird to declare those union types for
the sole purpose of using sizeof on them. I think it would
be cleaner to replace e.g. sizeof (phdr_u) with:
#define PHDR_MAX_SIZE MAX (sizeof (Elf32_Phdr), sizeof (Elf64_Phdr)
Prepare to compile without gnu99 extension.
A union like
{ T32 a32[n]; T64 a64[n]; } u;
is expanded to
void *data = malloc (...);
T32 *a32 = data;
T64 *a64 = data;
Signed-off-by: Chih-Hung Hsieh
---
libdwfl/dwfl_module_getdwarf.c | 46 +++-
libdwfl/dwfl_segment_rep
34 matches
Mail list logo
