[Sorry, I know this is long...] On Thu, Aug 23, 2018 at 10:11:22AM -0700, Kevin J. McCarthy wrote: > The calling function (e.g. _maildir_commit_message()) is not trying to > safe_rename() to the same filename over and over though. It is trying > to find an available target filename, and keeps modifying the target > filename using an incremented Counter and time(). So it makes sense for > it to retry if the target filename already existed. Or am I > misunderstanding your point?
OK. I haven't looked that closely at it (clearly).
In that case, it basically is analogous to mkstemp, i.e. it tries to
create a file with a particlar pattern, fails if it can't create it
safely, and retries with a different pattern. But those functions
will fail after TMP_MAX retries, and I think this code should do the
same. That prevents the DoS (infinite loop) and lets Mutt alert the
user that something funny is going on... This should never happen
unless the user is being attacked in some fashion.
On my system, TMP_MAX is defined as 238328. I think that's plenty of
retries! And since it's defined by the system, it's not exactly
arbitrary... =8^)
> > > Steffen's cautions apply to dotlock code, which is a different case and
> > > is not affected by this change.
> >
> > It's fundamentally the same thing though. The mechanism for dotlock
> > works like this:
> > [...]
> > If the link succeeds, we have the lock, but the rc from link is
> > unreliable, so...
>
> This is the important question. Under what circumstances is the rc from
> the link() unreliable? Are you saying a "0" success return value should
> not be trusted?
I read it as you do too, but 1. I think it's ambiguous enough that I
would not be 100% confident in that interpretation, and 2. this is for
Linux, and it's not clear that other platforms wouldn't have different
semantics (or even different file systems on Linux). I will note
that, for example, GnuPG always ignores the return value from link()
(common/dotlock.c:1070):
/* We ignore the return value of link() because it is unreliable. */
(void) link (h->tname, h->lockname);
if (stat (h->tname, &sb))
{
saveerrno = errno;
my_error_1 ("lock not made: Oops: stat of tmp file failed: %s\n",
strerror (errno));
/* In theory this might be a severe error: It is possible
that link succeeded but stat failed due to changed
permissions. We can't do anything about it, though. */
my_set_errno (saveerrno);
return -1;
}
> > - The stat comparison is supposed to catch the case where the return
> > value from link() incorrectly indicates failure, but it does not do
> > that because it only happens when link() succeeds. That defeats the
> > purpose of doing the check at all.
>
> No, the stat comparison was a double check for if the link __succeeds__.
That's not usually how it's done, as far as I can tell/remember. I
think I understand why, and I would refer you again to the GnuPG code
above as an example. It always ignores the link return value and does
the stat comparison.
Remember, the reason safe_rename() exists is because rename() has a
race condition on NFS--it is required to be atomic by the spec but on
NFS it is not. Therefore rename should be used only as a last resort,
and only if we're pretty sure link() actually failed.
If your link() return value is reliable, there's no reason to do the
stat comparison. If only the error case is unreliable, then on error
the stat comparison is NECESSARY, to determine if link() actually
succeeded despite the error. If neither is reliable, you should just
always ignore link()'s RV and always do the stat comparison.
So again, I think safe_rename() is wrong, regarless, and the change
you made actually has no practical effect. If it were doing the right
thing, your change would prevent it from catching the case where
> There is already failure handling code - reverting to trying a rename()
> for certain errno values. It was merely Vincent's hypothesis that the
> check was in the wrong place, but I don't believe that is correct.
Note that if the link() actually succeeded but incorrectly reported a
failure, the rename will also fail, in the sense that there will be
two links to the file, where the old link will not be removed. This
can be demonstrated on a local file system with the following short
program:
-=-=-=-=-
$ cat rename.c
#include <stdio.h>
#include <unistd.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <fcntl.h>
#include <stdlib.h>
char *old = "linkfile";
char *new = "linktarg";
int main(int argc, char **argv)
{
int fd;
// Show status of both files
system("ls -li linkfile linktarg");
// create the "old" file
printf("creating %s...\n", old);
if ((fd = open(old, O_CREAT, 0644)) == 1){
fprintf(stderr, "creating %s failed\n", old);
return 1;
}
close(fd);
system("ls -li linkfile linktarg");
// On a local file system, this should succeed
if (link(old, new) == -1) fprintf(stderr, "link fail\n");
else printf("link(%s, %s) succeeded.\n", old, new);
system("ls -li linkfile linktarg");
// simulate rename after incorrect link fail
if (rename(old, new) == -1) fprintf(stderr, "rename fail\n");
else printf("rename(%s, %s) succeeded.\n", old, new);
system("ls -li linkfile linktarg");
return 0;
}
$ ./rn
ls: cannot access 'linkfile': No such file or directory
ls: cannot access 'linktarg': No such file or directory
creating linkfile...
ls: cannot access 'linktarg': No such file or directory
27026462 -rw-r--r-- 1 demartin staff 0 Aug 23 14:50 linkfile
link(linkfile, linktarg) succeeded.
27026462 -rw-r--r-- 2 demartin staff 0 Aug 23 14:50 linkfile
27026462 -rw-r--r-- 2 demartin staff 0 Aug 23 14:50 linktarg
rename(linkfile, linktarg) succeeded.
27026462 -rw-r--r-- 2 demartin staff 0 Aug 23 14:50 linkfile
27026462 -rw-r--r-- 2 demartin staff 0 Aug 23 14:50 linktarg
So, I think this further supports what I'm saying. To restate my
various points more clearly, the algorithm for safe_rename() should be
this:
-=-=-=-
try a regular link and get its return value.
stat the old and new files, and compare. if the same, the link worked!
if they are different {
try rename.
if rename fails {
return failure.
}
}
unlink(old), ignore ENOENT, but report other errors.
-=-=-=-
The reason for the last is, the unlink() serves to clean up BOTH the
case where the link succeeded, AND the case where rename() succeeded
but didn't clean up the original link. However normally if rename
succeeds, the old link will get cleaned up, so unlink will fail in
that case, with ENOENT.
Alternatively, if you're convinced that the link() success status is
reliable, you can do the stat comparison conditionally, only if it
returns failure. Again, the reason to do this is to determine if the
link actually succeeded, but brokenly reported failure due to NFS
quirks.
The former (ignoring link's return value and always doing the stat
comparison) is probably safest for most users. The latter will
apparently be less problematic for SSHFS-like quirks, but as we saw
there's an SSHFS option that makes it work without this behavior.
> Your point about stat vs lstat sounds important, but let's first resolve
> whether the double-check needs to be restored or not.
Sure. Also note that rename() will possibly have different symlink
semantics, depending on which link() symlink behavior is in effect on
your platform. That may make this even more complicated.
Isn't POSIX fun? =8^)
--
Derek D. Martin http://www.pizzashack.org/ GPG Key ID: 0xDFBEAD02
-=-=-=-=-
This message is posted from an invalid address. Replying to it will result in
undeliverable mail due to spam prevention. Sorry for the inconvenience.
pgpI7oyxOY363.pgp
Description: PGP signature
