Re: pmachata/reader_hooks review

[Petr Machata <pmachata at redhat dot com>]

Roland McGrath wrote:
> __libdw_read_begin_end_pair_inc inverts the return value check on
> READ_AND_RELOCATE, no?  Since that's the only place that looks at
> the value of the expression, just make its value a "was relocated"
> Boolean (i.e. "status > 1").

Right, there was a bug there.

> I think we can just drop libdw_readhooks.c entirely for now.  Either make
> the two no-op stubs inlines in libdwP.h

Done.

> dwarf_formstring could use __libdw_formptr.

Well that gets rid of a bit of code in dwarf_formstring, but then 
__libdw_formptr has to handle two distinct cases: DW_FORM_strp, and 
DW_FORM_data* (when one is valid the other is not).  I just don't think 
it's worth it to add that kind of flexibility just for sake of one case.

> dwarf_getaranges and dwarf_getpubnames both had sanity checks on the
> offsets like "offset + 3 > d_size" (or 4).  Without these, a bogus offset
> could make them read off the end of the buffer in their fixed-size header
> reading bit.  Make read_offset take a "minimum to read" argument (pass 0 or
> 1 in the other places), so the consolidated check for bogus offsets can
> calculate that in.  Be sure those checks are immune from integer/pointer
> arithmetic overflow from bogus offsets, like:
> 	if (unlikely (offset > d_size) || unlikely (d_size - offset < minread))

I've rewritten the boundary checking logic to be like that.

> Are you about ready to move on to the testing regime?

I'm ready when you are.

PM

Attachment: signature.asc
Description: PGP signature