Re: [patch 2/2 v4] Fix loading core files without build-ids

[Mark Wielaard <mjw at redhat dot com>]

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
> about?

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.

Thanks,

Mark