Install should not follow the symlinks

[bapt]

Summary

install by default follows symlinks meaning if a symlink is replaced by
a file then the file targetted by the symlink is changed not the file

if the symlink was a dead symlink, then install fails

[bapt]

Note that this new behaviour matches the behaviour of GNU install

[bapt]

This approach is ok when replacing a normal symlink, but fails on a dead symlink returning "too many links"

[bapt]

Better approach:

Do not follow symlinks when checking if a target exists
Accept to replace a symlink with a file

[bapt]

Use a less confusing syntax

[bdrewery]

Hmm, my experience has been that install *does not* follow symlinks. I've relied on this at work in a very big way. Is your finding only for -l s?

[bapt]

In D4191#88148, @bdrewery wrote:

Hmm, my experience has been that install *does not* follow symlinks. I've relied on this at work in a very big way. Is your finding only for -l s?

Before my patch try
ln -sf ../nonexistent/foo bar
install /COPYRIGHT bar

and
touch something
ln -sf something bar
install /COPYRIGHT bar

now see the result you will see install does follow the symlinks

[bdrewery]

In D4191#88169, @bapt wrote:

In D4191#88148, @bdrewery wrote:

Hmm, my experience has been that install *does not* follow symlinks. I've relied on this at work in a very big way. Is your finding only for -l s?

Before my patch try
ln -sf ../nonexistent/foo bar
install /COPYRIGHT bar

and
touch something
ln -sf something bar
install /COPYRIGHT bar

now see the result you will see install does follow the symlinks

Hmm, it only "follows" for an error, it does not follow for the installation though.

root@bdrewery-fe3c5db-1:~/ > ln -sf /nonexistent/foo bar
root@bdrewery-fe3c5db-1:~/ > install /COPYRIGHT bar
install: bar: No such file or directory
root@bdrewery-fe3c5db-1:~/ > ls -ld bar
lrwxr-xr-x 1 root wheel 16 Nov 17 10:54 bar -> /nonexistent/foo
root@bdrewery-fe3c5db-1:~/ > ln -sf /blah bar
root@bdrewery-fe3c5db-1:~/ > touch /blah
root@bdrewery-fe3c5db-1:~/ > install /COPYRIGHT bar
root@bdrewery-fe3c5db-1:~/ > ls -ld bar
-rwxr-xr-x 1 root wheel 6142 Nov 17 10:55 bar
root@bdrewery-fe3c5db-1:~/ > cat /blah

Yes I would expect the first case to not hit an error.

[bapt]

Right sorry I miss explain the second case

ln -sf I_do_not_exists bar
install /COPYRIGHT bar

bar is still a symlink and I_do_not_exists has been created :)

[jhb]

It seems this implements Bryan's last suggestion of always overwriting a target symlink and never following it, correct?

I stopped looking after my few comments below. Figuring out the best solution to handle this edge case is needed though.

usr.bin/xinstall/xinstall.c

785 ↗

(On Diff #10267)

I think this if should fail if the target is a link? If the link happened to point to an identical fail the link would remain instead of being overwritten (and presumably it should be overwritten instead)?

if (docompare && !dostrip && target && S_IFREG(to_sb.st_mode))

837 ↗

(On Diff #10267)

Similarly here.

873 ↗

(On Diff #10267)

Not certain about this one. I almost wonder if it wouldn't be simpler to explicitly set target to -1 earlier for S_IFLINK? (Right after the EFTYPE error check.) The only sticky issue there might be if you need to ensure that the old link is explicitly removed later on.

[bdrewery]

Yes I would expect the first case to not hit an error.

FYI I was speaking only of my first call to install. I didn't try bapt's second example or look at it.

[bapt]

Update per @jhb comments

[bapt]

usr.bin/xinstall/xinstall.c

873 ↗

(On Diff #10276)

I think just making should we have a regular file is enough for here

[jhb]

usr.bin/xinstall/xinstall.c

873 ↗

(On Diff #10276)

This one in particular I am unsure of. I'm pretty sure the change here is wrong? I think it
should maybe be more like

if (dostrip && (!docompare || !target || !S_ISREG(to_sb.st_mode))

It might be clearer if it were redone as an 'else' to the clause above (if that's the correct logic) so that it was

if (docompare && dostrip && target && S_ISREG(to_sb.st_mode) {
    ....
} else if (dostrip)
    digestresult = digest_file(tempfile);

I'm just not sure if that is what it is trying to do. I'm surprised gcc/clang don't whine about digestresult possibly being used uninitialized. It's not obvious to me that it is always initialized.

[bapt]

Base on your comments I think this approach is even simpler and does what this
code is suppose to do

[jhb]

I think this is ok. I'd like to hear from Brooks if possible, but we can't wait forever. I feel like the digest bits should be simpler somehow so it is less convoluted.

[brooks]

I think either bapt or jhb's change is ok with regard to digestresult.

Everything about digest computation is terrible because it's over optimized to try and avoid reading the file twice.

The prior initialization of digestresult for the !dostrip case occurs either in the "If we don't strip, we can compare first." block or in the following "if (!files_match)" block. The sane thing to do would be to compute the digest IFF it's requested just before the call to metadata_log. If it wouldn't break everything I'd also love to kill the strip code. In the CheriBSD tree I no longer use install -s and strip as part of the build.