I made a somewhat reworked version of lbidiskfs/dir-renamed.c. This is the second attached patch. As you may guess, I prefer it over the other one.
But there is a much smaller patch that fixes the bug too. It is the first attached patch.
Regards -- Ognyan Kulev <[EMAIL PROTECTED]>, "\"Programmer\"" 7D9F 66E6 68B7 A62B 0FCF EB04 80BF 3A8C A252 9782
2003-06-11 Ognyan Kulev <[EMAIL PROTECTED]>
* dir-renamed.c (diskfs_rename_dir): Fixed assertion. Check
permissions to remove FROMNAME before any modification could take
place.
--- hurd/libdiskfs/dir-renamed.c.~1.22.~ 2001-10-12 05:49:17.000000000 +0300
+++ hurd/libdiskfs/dir-renamed.c 2003-06-11 18:46:21.000000000 +0300
@@ -1,5 +1,5 @@
/*
- Copyright (C) 1994,95,96,97,98,99,2001 Free Software Foundation, Inc.
+ Copyright (C) 1994,95,96,97,98,99,2001,03 Free Software Foundation, Inc.
This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License as
@@ -106,8 +106,15 @@
return 0;
}
- /* Now we can safely lock fnp */
- mutex_lock (&fnp->lock);
+ /* Check permissions to remove FROMNAME and lock FNP. */
+ tmpds = alloca (diskfs_dirstat_size);
+ err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, tmpds, fromcred);
+ assert (!tmpnp || tmpnp == fnp);
+ if (tmpnp)
+ diskfs_nrele (tmpnp);
+ diskfs_drop_dirstat (fdp, tmpds);
+ if (err)
+ goto out;
if (tnp)
{
@@ -199,8 +206,9 @@
ds = buf;
mutex_unlock (&fnp->lock);
err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, ds, fromcred);
- assert (tmpnp == fnp);
- diskfs_nrele (tmpnp);
+ assert (!tmpnp || tmpnp == fnp);
+ if (tmpnp)
+ diskfs_nrele (tmpnp);
if (err)
goto out;
2003-06-11 Ognyan Kulev <[EMAIL PROTECTED]>
* dir-renamed.c (diskfs_rename_dir): Reorder the statements so
that the whole function behave as atomic as possible when errors
occur.
(checkpath): Removed redundant statement.
--- hurd/libdiskfs/dir-renamed.c.~1.22.~ 2001-10-12 05:49:17.000000000 +0300
+++ hurd/libdiskfs/dir-renamed.c 2003-06-11 21:17:25.000000000 +0300
@@ -1,5 +1,5 @@
/*
- Copyright (C) 1994,95,96,97,98,99,2001 Free Software Foundation, Inc.
+ Copyright (C) 1994,95,96,97,98,99,2001,03 Free Software Foundation, Inc.
This program is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License as
@@ -32,9 +32,8 @@ checkpath(struct node *source,
error_t err;
struct node *np;
- np = target;
for (np = target, err = 0;
- /* nothing */;
+ np != diskfs_root_node;
/* This special lookup does a diskfs_nput on its first argument
when it succeeds. */
err = diskfs_lookup (np, "..", LOOKUP | SPEC_DOTDOT, &np, 0, cred))
@@ -50,13 +49,11 @@ checkpath(struct node *source,
diskfs_nput (np);
return EINVAL;
}
-
- if (np == diskfs_root_node)
- {
- diskfs_nput (np);
- return 0;
- }
}
+
+ /* We've reached the root node. */
+ diskfs_nput (np);
+ return 0;
}
/* Rename directory node FNP (whose parent is FDP, and which has name
@@ -72,45 +69,43 @@ diskfs_rename_dir (struct node *fdp, str
struct protid *fromcred, struct protid *tocred)
{
error_t err;
- struct node *tnp, *tmpnp;
- void *buf = alloca (diskfs_dirstat_size);
- struct dirstat *ds;
- struct dirstat *tmpds;
+ struct node *tnp, *fn_tmp, *fnddp;
+ struct dirstat *tn_ds = 0, *fndd_ds = 0, *fn_ds = 0;
+
+ /* 1: Prepare for modifications. Various checks are performed so
+ that as many errors as possible are catched before any change in
+ disk nodes. */
+ /* Is TDP child of FNP? */
mutex_lock (&tdp->lock);
diskfs_nref (tdp); /* reference and lock will get consumed by
checkpath */
err = checkpath (fnp, tdp, tocred);
-
if (err)
return err;
- /* Now, lock the parent directories. This is legal because tdp is not
- a child of fnp (guaranteed by checkpath above). */
+ /* Now, lock the parent directories. This is legal because TDP is not
+ a child of FNP (guaranteed by checkpath above). */
mutex_lock (&fdp->lock);
if (fdp != tdp)
mutex_lock (&tdp->lock);
- /* 1: Lookup target; if it exists, make sure it's an empty directory. */
- ds = buf;
- err = diskfs_lookup (tdp, toname, RENAME, &tnp, ds, tocred);
+ /* Get TONAME's node in TNP and its dirstat in TN_DS. */
+ tn_ds = alloca (diskfs_dirstat_size);
+ err = diskfs_lookup (tdp, toname, RENAME, &tnp, tn_ds, tocred);
assert (err != EAGAIN); /* <-> assert (TONAME != "..") */
if (tnp == fnp)
+ /* Source and target are the same node. */
{
- diskfs_drop_dirstat (tdp, ds);
- diskfs_nput (tnp);
- mutex_unlock (&tdp->lock);
- if (fdp != tdp)
- mutex_unlock (&fdp->lock);
- return 0;
+ fnp = 0; /* Don't unlock FNP as it's not locked yet. */
+ err = 0;
+ goto out;
}
- /* Now we can safely lock fnp */
- mutex_lock (&fnp->lock);
-
if (tnp)
{
+ /* If TONAME exists, then TNP must be empty directory. */
if (! S_ISDIR(tnp->dn_stat.st_mode))
err = ENOTDIR;
else if (!diskfs_dirempty (tnp, tocred))
@@ -118,64 +113,89 @@ diskfs_rename_dir (struct node *fdp, str
}
if (err && err != ENOENT)
- goto out;
+ {
+ fnp = 0;
+ goto out;
+ }
+
+ /* Get FROMNAME's dirstat in FN_DS. Lock FNP. */
+ fn_ds = alloca (diskfs_dirstat_size);
+ mutex_unlock (&fnp->lock);
+ err = diskfs_lookup (fdp, fromname, REMOVE, &fn_tmp, fn_ds, fromcred);
+ assert (!fn_tmp || fn_tmp == fnp);
+ if (fn_tmp)
+ diskfs_nrele (fn_tmp);
+ if (err)
+ {
+ fnp = 0;
+ goto out;
+ }
+ diskfs_drop_dirstat (fdp, fn_ds);
+ fn_ds = 0;
- /* 2: Set our .. to point to the new parent */
if (fdp != tdp)
+ /* We'll move across directories. */
{
- if (tdp->dn_stat.st_nlink == diskfs_link_max - 1)
+ /* Get FNP's ".." dirstat in FNDD_DS. */
+ fndd_ds = alloca (diskfs_dirstat_size);
+ err = diskfs_lookup (fnp, "..", RENAME | SPEC_DOTDOT,
+ &fnddp, fndd_ds, fromcred);
+ assert (err != ENOENT);
+ if (err)
+ goto out;
+ assert (fnddp == fdp);
+
+ if (tdp->dn_stat.st_nlink == diskfs_link_max - 1
+ || fnp->dn_stat.st_nlink == diskfs_link_max - 1)
+ /* TDP can't afford another link (by FNP) to it,
+ or FNP can't afford another link (by TDP) to it. */
{
err = EMLINK;
- return EMLINK;
+ goto out;
}
+ }
+
+ /* 2: Modify disk nodes to match the new state. */
+
+ if (fdp != tdp)
+ /* Set FNP's .. to point to the new parent TDP. */
+ {
+ /* TDP has new child (FNP). */
tdp->dn_stat.st_nlink++;
tdp->dn_set_ctime = 1;
if (diskfs_synchronous)
diskfs_node_update (tdp, 1);
- tmpds = alloca (diskfs_dirstat_size);
- err = diskfs_lookup (fnp, "..", RENAME | SPEC_DOTDOT,
- &tmpnp, tmpds, fromcred);
- assert (err != ENOENT);
- if (err)
- {
- diskfs_drop_dirstat (fnp, tmpds);
- goto out;
- }
- assert (tmpnp == fdp);
-
- err = diskfs_dirrewrite (fnp, fdp, tdp, "..", tmpds);
+ /* FNP has new parent (TDP). */
+ err = diskfs_dirrewrite (fnp, fdp, tdp, "..", fndd_ds);
+ fndd_ds = 0;
if (diskfs_synchronous)
diskfs_file_update (fnp, 1);
- if (err)
- goto out;
+ /* FNP is no longer child of FDP. */
fdp->dn_stat.st_nlink--;
fdp->dn_set_ctime = 1;
if (diskfs_synchronous)
diskfs_node_update (fdp, 1);
+
+ if (err)
+ goto out;
}
+ /* Increment the link count on the node being moved and rewrite
+ TDP. */
- /* 3: Increment the link count on the node being moved and rewrite
- tdp. */
- if (fnp->dn_stat.st_nlink == diskfs_link_max - 1)
- {
- mutex_unlock (&fnp->lock);
- diskfs_drop_dirstat (tdp, ds);
- mutex_unlock (&tdp->lock);
- if (tnp)
- diskfs_nput (tnp);
- return EMLINK;
- }
+ /* Add FNP to TDP. */
+ /* XXX ogi: Why is this needed when below st_nlink is decremented? */
fnp->dn_stat.st_nlink++;
fnp->dn_set_ctime = 1;
diskfs_node_update (fnp, 1);
if (tnp)
{
- err = diskfs_dirrewrite (tdp, tnp, fnp, toname, ds);
- ds = 0;
+ /* Replace TNP with FNP. */
+ err = diskfs_dirrewrite (tdp, tnp, fnp, toname, tn_ds);
+ tn_ds = 0;
if (!err)
{
tnp->dn_stat.st_nlink--;
@@ -187,7 +207,9 @@ diskfs_rename_dir (struct node *fdp, str
}
else
{
- err = diskfs_direnter (tdp, toname, fnp, ds, tocred);
+ /* FNP is new child of TDP. */
+ err = diskfs_direnter (tdp, toname, fnp, tn_ds, tocred);
+ tn_ds = 0;
if (diskfs_synchronous)
diskfs_file_update (tdp, 1);
}
@@ -195,17 +217,25 @@ diskfs_rename_dir (struct node *fdp, str
if (err)
goto out;
- /* 4: Remove the entry in fdp. */
- ds = buf;
+ /* Get FROMNAME's dirstat in FN_DS. XXX: This is the same code as
+ in the preparation. The reason is that the information taken
+ before is unreliable but it serves to check permissions. */
+ fn_ds = alloca (diskfs_dirstat_size);
mutex_unlock (&fnp->lock);
- err = diskfs_lookup (fdp, fromname, REMOVE, &tmpnp, ds, fromcred);
- assert (tmpnp == fnp);
- diskfs_nrele (tmpnp);
+ err = diskfs_lookup (fdp, fromname, REMOVE, &fn_tmp, fn_ds, fromcred);
+ assert (!fn_tmp || fn_tmp == fnp);
+ if (fn_tmp)
+ diskfs_nrele (fn_tmp);
if (err)
- goto out;
+ {
+ fnp = 0;
+ goto out;
+ }
- diskfs_dirremove (fdp, fnp, fromname, ds);
- ds = 0;
+ /* Remove FNP from FDP. */
+ err = diskfs_dirremove (fdp, fnp, fromname, fn_ds);
+ assert (!err);
+ fn_ds = 0;
fnp->dn_stat.st_nlink--;
fnp->dn_set_ctime = 1;
if (diskfs_synchronous)
@@ -214,6 +244,8 @@ diskfs_rename_dir (struct node *fdp, str
diskfs_node_update (fnp, 1);
}
+ /* 3: Cleanups when all is over or error has occurred. */
+
out:
if (tdp)
mutex_unlock (&tdp->lock);
@@ -223,7 +255,11 @@ diskfs_rename_dir (struct node *fdp, str
mutex_unlock (&fdp->lock);
if (fnp)
mutex_unlock (&fnp->lock);
- if (ds)
- diskfs_drop_dirstat (tdp, ds);
+ if (tn_ds)
+ diskfs_drop_dirstat (tdp, tn_ds);
+ if (fn_ds)
+ diskfs_drop_dirstat (tdp, fn_ds);
+ if (fndd_ds)
+ diskfs_drop_dirstat (tdp, fndd_ds);
return err;
}
_______________________________________________ Bug-hurd mailing list [EMAIL PROTECTED] http://mail.gnu.org/mailman/listinfo/bug-hurd
