This is the mail archive of the
mailing list for the elfutils project.
Re: [patch 2/2 v4] Fix loading core files without build-ids
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Thu, 30 May 2013 11:52:55 +0200
- Subject: Re: [patch 2/2 v4] Fix loading core files without build-ids
On Wed, 2013-05-29 at 17:57 +0200, Jan Kratochvil wrote:
> On Wed, 29 May 2013 11:50:26 +0200, Mark Wielaard wrote:
> > On Tue, 2013-05-28 at 20:36 +0200, Jan Kratochvil wrote:
> > > But when we talk about it in general, as a new patchset, I rather find a bug
> > > dropping the path, when core file already contains the absolute pathname why
> > > to hide it from the user?
> > Right, if we can guarantee that path is the real one and not just an
> > arbitrary guess.
> It always was the real pathname - why it would not be? Or what have I forgot
You forgot Nothing. I was just not very clear. I just meant that the
full path is there in eu-unstrip -n output if the actual file on the
file system is the one matching. It is now just missing in the output if
the file on the system is not a match. But that is fine.
> > > + if (module_start <= module->l_ld && module->l_ld < module_end)
> > Should the fixup not always be calculated and then the next check takes
> > care of everything?
> I do not find that safe; this could match unrelated L_LD from the other end of
> address space just due to a "luck" it has the same '% PAGE_SIZE' offset.
> IIUC MODULE_START and MODULE_END are always safely located inside the segment
> (considering they have BIAS already added); the segment address may be wrong
> but it only matters there cannot be other shared library located at the same
> address range. L_LD is also always safe as it comes from link map. Therefore
> the first conditional is always safe:
> if (module_start <= module->l_ld && module->l_ld < module_end)
> I cannot much imagine how the second conditional after FIXUP adjustment
> && module_start + fixup <= module->l_ld
> && module->l_ld < module_end + fixup)
> could fail but it is just a sanity check of FIXUP.
> This is why I believe the first conditional should be there; the second
> conditional may be redundant but it cannot hurt.
> Or do you see some inconsistency in my safe / unsafe considerations above?
No, that seems logical. Thanks.
> > > + GElf_Addr fixup = module->l_ld - dyn_vaddr;
> > > + if ((fixup & (dwfl->segment_align - 1)) == 0
> > > + && module_start + fixup <= module->l_ld
> > > + && module->l_ld < module_end + fixup)
> > Since the fixup could just be zero?
> FIXUP will be in 99% of cases zero, what's the problem with it? I did not get
> your comment here.
There is no problem. As you said above the extra check might be
redundant. But that is fine.
> > Looks good to me. But please merge this commit and the code movement
> > commit into one, since those are really one change.
> Yes, sure, it would break git bisect as it does not build with just the first
> part applied.
> Not checked in yet until the discussion gets finished.
I think it is finished now.