Re: [patch] install with a missing strip tool

[Ondrej Oprala]

On 02/21/2013 05:46 PM, Bernhard Voelker wrote:
> On 02/21/2013 04:44 PM, Ondrej Oprala wrote:
> > Hi,
> >    this patch address the problem mentioned here by John Reiser:
> > https://bugzilla.redhat.com/show_bug.cgi?id=632444  .
> > Install now checks the $PATH or the path given for the strip
> > program and turns off the -s option if none is found.
> >
> > Thanks,
> > Ondrej.
> Hi Ondrej,
>
> thanks for working on this old bug report.
>
> > +  if (strip_files)
> > +    {
> > +      /* if specified with a path.  */
> > +      if (strip_program_specified && strchr (strip_program, '/'))
> Doesn't this hard-coded directory separator make porting
> difficult (e.g. to Cygwin)?
>
> > +        {
> > +          struct stat dst_st;
> > +          if (lstat (strip_program, &dst_st) == -1)
> What if strip_program is dangling symlink? Then lstat will
> succeed but the installed program will also be left behind
> with wrong permissions (yet ginstall would indicate this failure
> with a proper diagnostic, as today).
>
> > +            strip_files = false;
> > +        }
> > +      else
> > +        {
> > +          char *path = (char *)find_in_path (strip_program);
> A little and very unlikely race - if the strip_program is removed
> between the call of find_in_path () and strip (), but well ...
>
> > +          if (STREQ (path, strip_program))
> > +            strip_files = false;
> > +          else
> > +            free (path);
> > +        }
> > +      if (!strip_files)
> > +        error (0, 0, _("WARNING: %s not found - ignoring the -s option"),
> > +              quote (strip_program));
> > +    }
> In general, I'm a bit biased about the solution: wouldn't it
> be better to simply "unlink (name)" if the strip_program failed?
> This would be also safer because the strip_program might fail due
> to other reasons (system limits, bad ELF format, ...). The bare
> check if the file can be found (even in PATH) and is executable is
> not sufficient IMO.
>
> Have a nice day,
> Berny
Hi Bernhard,
thanks for looking at the patch; it is, of course, possible to unlink 
the file if
the strip itself fails, which seems like an easier solution as well. To 
be honest I like the idea
of checking for strip more than just reverting the changes if something 
goes wrong,
but you raise some valid points in my code and I don't see a reasonable 
way to fix
all of them (and I doubt it would be worth the effort) , so let's do 
your approach ;) .

Thanks,
Ondrej

DIFF
Description: Text document