rename: it was writing v1 dirents into a B-tree, and nobody noticed

`rename()` refused an existing destination, so write-to-a-temporary-then-rename
-- how every careful program saves a file -- failed on the second save of
anything. zsh's history was the visible case: `.zsh_history.new` accumulated
for ever and the history itself never updated.

An earlier attempt to fix this was reverted after it lost both files, and the
note it left behind blamed HTree slot reuse. That was the wrong suspect.

**sys_rename was removing the old name by hand.** memset a `struct dirent` to
zero and writei() it into the parent directory at the offset dirlookup()
returned. That is the v1 filesystem's on-disk layout, written directly,
bypassing dp->ops entirely. On SchFS a directory is a B-tree with an index over
it -- "the record at that offset" is not a thing, and writing zeroes there
corrupted the directory while leaving the name in the index. Adding a second
removal on top of a corrupting one is how both files disappeared.

Every other caller in the file already used dirunlink(). This one had been
open-coding a filesystem it no longer ran on, and the bug stayed invisible for
as long as nothing asked it to remove two names instead of one.

With that corrected, replacing the destination is the straightforward thing it
should always have been: look it up, refuse if it is a directory, unlink it,
drop its nlink. rename(a, a) returns 0 and touches nothing, per POSIX -- both
names already reach the same file, and removing one to add it back is only
equivalent when nothing else is watching.

The lock discipline is inherited rather than invented, and it is the delicate
part: dp_old and dp_new are frequently the *same* directory reached through two
nameiparent() calls, which is two references to one inode. So no two directory
locks are held at once -- each is locked, used, unlocked before the next -- and
the source inode stays unlocked throughout, held alive by an nlink bump.

Directories are still refused, in both roles: moving one needs the `..` fixup
and the ancestor-loop check that are not written, and replacing one needs them
plus an emptiness test. EISDIR either way.

    mv a c        ->  0, c has a's contents
    mv c b        ->  0, b replaced, b has a's contents   (this is the fix)
    mv b b        ->  0, nothing happens
    mv nope z     ->  ENOENT
    mv b d/       ->  0, b lands inside d

60 create-and-rename cycles leave one file and 4 KB of disk, not 60. /root
holds .zsh_history and no .zsh_history.new, and a marker written in one boot is
still in the history file on the next.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Nexoniarz 2026-09-20 01:33:15 +02:00
commit 370605452c
2 changed files with 102 additions and 18 deletions

View file

@ -874,13 +874,37 @@ addr_t sys_truncate(void)
return 0;
}
// rename(2).
//
// Two things were wrong here and only one of them showed.
//
// **The old name was removed by hand.** `memset(&de, 0, ...)` then a writei()
// straight into the parent directory's data at the offset dirlookup() gave
// back. That is the v1 filesystem's on-disk layout -- an array of dirents --
// written directly, bypassing dp->ops. On SchFS a directory is a B-tree with
// an HTree index over it, so zeroing "the record at that offset" wrote into a
// structure that does not work that way and left the name in the index. Every
// other caller in this file uses dirunlink(); this one had been open-coding a
// filesystem it no longer ran on.
//
// **And an existing destination was refused.** POSIX says rename() replaces
// it atomically, and the one program everybody notices this with is a shell
// saving history: zsh writes $HISTFILE.new and renames it over $HISTFILE, so
// a rename that cannot replace leaves a .zsh_history.new behind after every
// session and never updates the history at all.
//
// The lock discipline is the delicate part, and it is inherited rather than
// invented: dp_old and dp_new are frequently the *same* directory reached
// through two nameiparent() calls, which is two references to one inode. So
// no two directory locks are ever held at once -- each is locked, used, and
// unlocked before the next -- and `ip` is unlocked throughout, held alive by
// the nlink bump instead.
addr_t sys_rename(void)
{
char *oldpath, *newpath;
struct inode *ip, *dp_old, *dp_new, *ip_src;
struct inode *ip, *dp_old, *dp_new, *ip_src, *ip_dst;
char oldname[DIRSIZ], newname[DIRSIZ];
uint off;
struct dirent de;
uint off, dstoff;
int err = EPERM;
if (argstr(0, &oldpath) < 0 || argstr(1, &newpath) < 0) return -EFAULT;
begin_op();
@ -898,7 +922,34 @@ addr_t sys_rename(void)
if (dp_new) iput(dp_new);
goto bad_rename;
}
// Clear the destination, if anything is there.
ilock(dp_new);
if ((ip_dst = dirlookup(dp_new, newname, &dstoff)) != 0) {
// Both names already reach the same file. POSIX: succeed and do
// nothing -- which includes not unlinking either name, since removing
// one and adding it back is only equivalent when nothing else is
// watching.
if (ip_dst->inum == ip->inum) {
iput(ip_dst); iunlockput(dp_new); iput(dp_old);
ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip);
end_op(); return 0;
}
ilock(ip_dst);
// Replacing a directory would need it to be empty and would need the
// parent's link count fixed up. Refusing is honest; the directory case
// of rename is unimplemented as a whole.
if (ip_dst->type == T_DIR) {
iunlockput(ip_dst); iunlockput(dp_new); iput(dp_old);
err = EISDIR; goto bad_rename;
}
if (dirunlink(dp_new, newname, dstoff) < 0) {
iunlockput(ip_dst); iunlockput(dp_new); iput(dp_old);
err = EIO; goto bad_rename;
}
ip_dst->nlink--; iupdate(ip_dst); iunlockput(ip_dst);
}
if (dirlink(dp_new, newname, ip->inum) < 0) {
iunlockput(dp_new); iput(dp_old); err = EEXIST; goto bad_rename;
}
@ -909,8 +960,10 @@ addr_t sys_rename(void)
end_op(); return -ENOENT;
}
iput(ip_src);
memset(&de, 0, sizeof(de));
if (writei(dp_old, 0, (uint64)&de, off, sizeof(de)) != sizeof(de)) panic("rename: writei");
// Through the filesystem, not through writei(). See the note above.
if (dirunlink(dp_old, oldname, off) < 0) {
iunlockput(dp_old); err = EIO; goto bad_rename;
}
iunlockput(dp_old);
ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip);
end_op(); return 0;