Pádraig Brady <[email protected]> writes:
> On 05/11/2026 05:56, Collin Funk wrote:
>
>> diff --git a/NEWS b/NEWS
>> index ae5c8e32a..14b1206dc 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -95,6 +95,14 @@ GNU coreutils NEWS -*-
>> outline -*-
>> before invoking 'strip' instead of using a umask of zero.
>> [This bug was present in "the beginning".]
>>
>> + 'install -D --strip' runs the strip program from the current working
>> directory
>> + when the source and destination are absolute paths. Previously, it would
>> run
>> + the strip program from the parent directory of the destination file, which
>> + could lead to executing the incorrect program if the PATH environment
>> variable
>> + was set to an empty string or a relative file name was given to the
>> + --strip-program option.
>> + [bug introduced in coreutils-6.2]
>
>
> That was a bit confusing to me, also $PATH only needs to have an empty entry,
> not be fully empty. I'd go with this:
>
> 'install -D --strip' now runs the strip program based in the initial
> working
> directory, even when the source and destination are absolute paths.
> Previously, it would run the strip program from the parent directory of the
> destination file, which could execute the incorrect program if $PATH
> allowed
> the current directory, or --strip-program specified a relative path.
> [bug introduced in coreutils-6.2]
Sounds good; I changed it to that. I try to keep NEWS entries short and
to the point, but in this case it was almost impossible not to write a
wall of text. Perhaps I was too focused on that.
> I was wondering why this didn't do:
>
> (strip_files && !IS_ABSOLUTE_FILE_NAME (split_program))
>
> Though I suppose an absolute split program could load
> config files or whatever relative to the current dir,
> so it's more consistent to always run strip from the initial dir.
To be honest, I did not think about 'strip' loading config files and
such. I considered using "!IS_ABSOLUTE_FILE_NAME (split_program)" as you
mentioned, but you would also have to check for an empty entry in $PATH.
It felt a bit bug-prone to me, and I didn't like adding another getenv
call on top of the ones used for posix_spawnp. I didn't see much benefit
from it to make it worth it.
> The comment doesn't mention any of that, and only stated
> the obvious to me, so therefore I'd replace the 5 line comment like:
>
> diff --git a/src/install.c b/src/install.c
> index 13621574c..06a51e5b2 100644
> --- a/src/install.c
> +++ b/src/install.c
> @@ -762,13 +762,8 @@ static bool
> mkancesdirs_safe_wd (char const *from, char *to, struct cp_options *x,
> bool save_always)
> {
> - /* Check whether we have to save the current working directory.
> - When using --strip, we must save the current working directory
> - since STRIP_PROGRAM may be relative or $PATH may be set to an
> - empty string, meaning STRIP_PROGRAM may be found and executed
> - in the current working directory. */
> bool save_working_directory =
> - (save_always || strip_files
> + (save_always || strip_files /* strip may be, or access relative files.
> */
> || ! (IS_ABSOLUTE_FILE_NAME (from) && IS_ABSOLUTE_FILE_NAME (to)));
>
> struct savewd wd;
True, I guess it sort of just re-explains what savewd does. That module
always felt a bit tricky for me to understand, probably because I began
programming when openat was long invented. That is my excuse for the
long comment, at least. :)
Thanks for the review, I pushed it with those changes.
Collin
