[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1] dwarf-reader: fix undefined behaviour in get_binary_load_address
Hi Dodji!
On Thu, Apr 18, 2019 at 11:59:07AM +0200, Dodji Seketeli wrote:
> "Matthias Maennich via libabigail" <libabigail@sourceware.org> a écrit:
>
> > Refactor the method to just keep track of the minimal address without
> > doing so through a pointer to GElf_Phdr.
>
> The logic of the refactoring seems correct to me.
>
> However, one thing to keep in mind is that unfortunately, the code of
> libabigail is still in C++ < 11 (effectively c++98) because at the time,
> it was meant to also run on distributions that didn't have C++11 yet,
> and some of these distributions are supported for a long time. They
> thus expect libabigail to still be running there. Time is coming
> though, for us to move to C++ >= 11, I think :-) But we haven't flipped
> that switch yet. Until then, I think we need to stick to C++ < 11.
>
> Therefore ...
>
> [...]
>
> > + bool address_found = false;
> > + GElf_Addr result = std::numeric_limits<GElf_Addr>::max();
>
> This change is not yet acceptable as std::numeric_limits doesn't exist
> pre C++11.
>
Actually, (I had to look it up first) std::numeric_limits is already part of
C++98 ([lib.limits/18.2.1]). So, I guess we can use that then?
By the way, any concrete plans to introduce C++11 or later? Just curious.
> I thus went ahead and edited your patch to keep the minimal amount of
> change that would still fix the issue you are validly raising in the
> subject of the patch.
>
Thanks for taking care of that. I have one more question about that. As far as
I understand, `ph_mem` gets updated unconditionally during every iteration via
the call to `gelf_getphdr`. Also, `program_header` will always be `&ph_mem` as
that is what `gelf_getphdr` returns. So, once we update
`lowest_program_header`, it will be `&ph_mem`. And if we update `ph_mem` in the
next iteration, we effectively update `*lowest_program_header`. Is that the
intention? My original patch was therefore keeping track of the minimum
`program_header.p_vaddr` value seen in case of `program_header.p_type ==
PT_LOAD`. Maybe I am wrong, but can you double check the logic?
--
Cheers,
Matthias