[Toybox] ln.c patch (was Re: [Patches] - Issue fixes)

[Rob Landley]

On 10/16/14 01:31, Ashwini Sharma wrote:
> Hi Rob, List,
> 
> Few more issues observed.  Please find attached the fix patches.

Sorry this is taking so long, but each one of these takes a bit of scrutiny
to get right, and can turn into something like the below:

> ln.c : with __-f__ with an existing destination unlink was done,
>        but in case where the requested operation failed, the original
>        file is not restored.
>        e.g.  ln -f dir/ lnk, where source is dir and dest lnk existed.


Because that's what posix ln spec section 1(c) said to d. That said, if
you actually want recovery code we can add it. (Although I think it can
only fail in this way if the target OS doesn't support symlinks or if we've
run out of inodes? And running out of inodes would require an asynchronous
process consuming them between the free and the reuse, or possibly a
"filesystem full" case where the symlink text doesn't fit in the dentry
we just freed up with the rm, and requires allocating space?)

Ah, hardlink across filesystems. Ok, there's a real world test case for this.

However, the recovery code is itself on the brittle side, in a number of
ways. For example this new code:

+    if (rc) {
+      perror_msg("cannot create %s link from '%s' to '%s'",
         (toys.optflags & FLAG_s) ? "symbolic" : "hard", try, new);
+      if (toys.optflags & FLAG_f) toys.exitval = rename(tmp, new);
+    } else {

You fail, and calling perror_msg() sets toys.exitval to 1 if it was
previously zero. But then when you rename the old file back, you save the
result in toys.exitval so if the rename worked you exit "success".

At a design level, if your mkstemp fails and then the symlink()/link() fails,
we try to rename() the _XXXXXX name over the original file. (Probably harmless.)
If the original rename() fails (we don't even check that) and then the symlink
fails, we rename() an empty file over the original file. (Not harmless, although
not necessarily possible to trigger either...)

Reading the posix spec, another problem we're supposed to catch is
"ln -f samefile samefile". Then again, we _do_ fail: saying the destination
file exists (without -f) or that the source file doesn't exist (with -f).
This doesn't actually change the behavior, and posix doesn't specify what
the diagnostic message should _be_, so I guess we're ok...

Here's another failure mode:

  mkdir -p one/one/blah
  ln -sf /bin/one one

With the old code the unlink fails loudly (can't overwrite one/one). With the
new code it does the rename, then creates the new entry, and then fails to
delete the renamed file. (Exiting with a nonzero error code, but no _message_.)

Possibly what we should do is create the new entry _at_ the temporary name
and then try to rename() it into place, and then if it fails we should always
be able to delete the temp (because we just created it, belongs to us,
can't be a directory because hardlinks to directories aren't allowed...).

Hmmm... the existing "stat destination and xmprintf dir/file" logic means
we don't have to worry about the rename being accross a mount point (because
we can create the tempfile _in_ the directory), but in the above "another
failure mode" test case it would create "one/one_ABCDEF" and then the rename
to "one/one" might put it in "one/one/one". Which is not the intended behavior.
The rename should fail instead of putting it _in_ a directory.

Ah, no the rename() man page says it'll -EISDIR in that case. Ok, this
should work.

It would be really nice to add a bunch of new tests to tests/ln.test to
cover all the cases mentioned in this email. (Todo item: go through the
list archives, and my blog, and create tests for every weird corner case
I've complained about over the years.)

Checked in a fix, might even be right. :) It passes the test suite, for
what that's worth...

Rob

 1414035687.0