Re: [patch] Fix for PIE with both -e and --core in use

[Mark Wielaard <mjw at redhat dot com>]

On Tue, 2012-11-27 at 19:50 +0100, Jan Kratochvil wrote:
> updated:
> jankratochvil/pie-core

Thanks.

> Maybe there would be some way to 'patch' the former stale segment but this is
> IMO exactly what the patch does, via that 'find_elf' callback.

I thought about it, but couldn't think of another way than how your
patch does it.

> > It took me a while to realize this is what the patch seems to do since the
> > documentation for the find_elf callback doesn't say that will happen when
> > you do that (actually, there sadly is no documentation at all for this
> > callback...).
> 
> Sorry, I tried to match how elfutils is written.

No worries, it is consistent with how other parts of the code do it. It
was just surprising to me because I had assumed returning -1 would
indicate a general error/cannot do it. We should just update the
documentation a bit. There are a couple of places where some extra (or
actually any) documentation on the callbacks would be very helpful,
especially how the return values are interpreted. I'll try and write
something based on what I learned from reviewing your patch.

> > > +	    if (opt->e)
> > > +	      for (Dwfl_Module *mod = dwfl->modulelist; mod; mod = mod->next)
> > > +		{
> > > +		  if (mod->name == NULL
> > > +		      || (strcmp (mod->name, "[exe]") != 0
> > > +			  && strcmp (mod->name, "[pie]") != 0))
> > > +		    continue;
> > > +		  mod->userdata = (void *) opt->e;
> > > +		}
> > 
> > Why is this done?
> 
> So that offline_find_elf can find the executable name.

Ah, ok, I misread the patch, I thought you used the argp_state for that,
but of course that isn't available when offline_find_elf () is called.

> Global variable is unsafe as the caller can call it for arbitrary number of
> inputs concurrently so where to put it otherwise?

Is that really true? See below.

> > The following documentation in dwflp.h doesn't really make that clear.
> > Also it doesn't mention this is only done when --core is given.
> > 
> > > -/* Standard argument parsing for using a standard callback set.  */
> > > +/* Standard argument parsing for using a standard callback set.
> > > +   Field Dwfl_Module->USERDATA is reserved for "[exe]" or "[pie]" modules.  */
> > >  struct argp;
> > >  extern const struct argp *dwfl_standard_argp (void) __attribute__ ((const));
> 
> I could comment it otherwise but I have created a new Dwfl_module field
> instead.  It was too fragile to use USERDATA, in fact it made USERDATA
> unavailable for anything else from applications.

I think it would be better to use a global static in argp-std.c.
dwfl_standard_argp () returns a global static argp pointer, so it cannot
really be reused anyway and there can be only one executable per
Dwfl/argp. You do allocate the parse_opt dynamically, but it looks to me
that it could just as well be static like libdwfl_argp. Or am I missing
a use case?

Thanks,

Mark