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
From 154ca82e4436cc4ae30c647ab0cade5f909f7d3f Mon Sep 17 00:00:00 2001
From: Ondrej Oprala <[email protected]>
Date: Thu, 21 Feb 2013 20:06:03 +0100
Subject: [PATCH] install: cleanup properly if the strip program failed for
any reason
* src/install.c (strip): Indicate failure with a return code instead
of terminating the program.
(install_file_in_file): Handle strip's return code and unlink the
created file if necessary.
* tests/install/strip-program.sh: Add a test to cover the changes.
---
src/install.c | 21 +++++++++++++++++----
tests/install/strip-program.sh | 5 +++++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/src/install.c b/src/install.c
index 94374df..904c0ea 100644
--- a/src/install.c
+++ b/src/install.c
@@ -25,6 +25,7 @@
#include <grp.h>
#include <selinux/selinux.h>
#include <sys/wait.h>
+#include <unistd.h>
#include "system.h"
#include "backupfile.h"
@@ -515,7 +516,7 @@ change_timestamps (struct stat const *src_sb, char const
*dest)
magic numbers vary so much from system to system that making
it portable would be very difficult. Not worth the effort. */
-static void
+static int
strip (char const *name)
{
int status;
@@ -532,11 +533,18 @@ strip (char const *name)
break;
default: /* Parent. */
if (waitpid (pid, &status, 0) < 0)
- error (EXIT_FAILURE, errno, _("waiting for strip"));
+ {
+ error (0, errno, _("waiting for strip"));
+ return -1;
+ }
else if (! WIFEXITED (status) || WEXITSTATUS (status))
- error (EXIT_FAILURE, 0, _("strip process terminated abnormally"));
+ {
+ error (0, 0, _("strip process terminated abnormally"));
+ return -1;
+ }
break;
}
+ return 0;
}
/* Initialize the user and group ownership of the files to install. */
@@ -681,7 +689,12 @@ install_file_in_file (const char *from, const char *to,
if (! copy_file (from, to, x))
return false;
if (strip_files)
- strip (to);
+ if (strip (to))
+ {
+ if (unlink (to))
+ error (EXIT_FAILURE, errno, _("cannot unlink %s"), to);
+ return false;
+ }
if (x->preserve_timestamps && (strip_files || ! S_ISREG (from_sb.st_mode))
&& ! change_timestamps (&from_sb, to))
return false;
diff --git a/tests/install/strip-program.sh b/tests/install/strip-program.sh
index 8950d50..1ee4a7a 100755
--- a/tests/install/strip-program.sh
+++ b/tests/install/strip-program.sh
@@ -33,4 +33,9 @@ echo aBc > exp || fail=1
ginstall src dest -s --strip-program=./b || fail=1
compare exp dest || fail=1
+#check that install cleans up properly if strip fails
+
+ginstall src dest -s --strip-program=./FOO &>/dev/null && fail=1
+test -e dest && fail=1
+
Exit $fail
--
1.7.11.7
