On 02/21/2013 08:40 PM, Ondrej Oprala wrote: > so let's do your approach ;)
Hi Ondrej, thank you. In your patch, you missed the case when fork failed: the target would yet be left behind. Therefore, and to have slightly smaller code, I inverted the logic in strip(), and used a bool return value instead of int. The inclusion of unistd.h was unnecessary BTW: maint.mk: the above are already included via system.h Since this was reported in Redhat's Bugzilla, and it changes the behavior in the case of such an error for the user, I've also added a NEWS entry. Finally, I've removed the error suppression in the test, as it may help to diagnose problems in future. I also changed the target file name to avoid interference with the first test (paranoia, I know... ;-) Is this okay for you? Have a nice day, Berny
>From 43d27a5338d2ada5b7a4328ece19e894330acdf3 Mon Sep 17 00:00:00 2001 From: Ondrej Oprala <[email protected]> Date: Fri, 22 Feb 2013 13:48:57 +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. * NEWS (Bug fixes): Mention the fix. Reported by John Reiser in http://bugzilla.redhat.com/632444. --- NEWS | 7 +++++++ src/install.c | 19 ++++++++++++++----- tests/install/strip-program.sh | 4 ++++ 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/NEWS b/NEWS index 37bcdf7..5a25377 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,13 @@ GNU coreutils NEWS -*- outline -*- * Noteworthy changes in release ?.? (????-??-??) [?] +** Bug fixes + + install now removes the target file if the strip program failed for any + reason. Before, that file was left behind, sometimes even with wrong + permissions. + [This bug was present in "the beginning".] + * Noteworthy changes in release 8.21 (2013-02-14) [stable] diff --git a/src/install.c b/src/install.c index 94374df..a5ed7a8 100644 --- a/src/install.c +++ b/src/install.c @@ -515,16 +515,17 @@ 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 bool strip (char const *name) { int status; + bool ok = false; pid_t pid = fork (); switch (pid) { case -1: - error (EXIT_FAILURE, errno, _("fork system call failed")); + error (0, errno, _("fork system call failed")); break; case 0: /* Child. */ execlp (strip_program, strip_program, name, NULL); @@ -532,11 +533,14 @@ 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")); else if (! WIFEXITED (status) || WEXITSTATUS (status)) - error (EXIT_FAILURE, 0, _("strip process terminated abnormally")); + error (0, 0, _("strip process terminated abnormally")); + else + ok = true; /* strip succeeded */ break; } + return ok; } /* Initialize the user and group ownership of the files to install. */ @@ -681,7 +685,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) != 0) /* Cleanup. */ + 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..5d65373 100755 --- a/tests/install/strip-program.sh +++ b/tests/install/strip-program.sh @@ -33,4 +33,8 @@ 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 dest2 -s --strip-program=./FOO && fail=1 +test -e dest2 && fail=1 + Exit $fail -- 1.8.1.3.619.g7b6e784
