Re: [patch] install with a missing strip tool

[Bernhard Voelker]

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