Bug#190732: [PATCH] hurd/libdiskfs/dir-renamed.c

[Ognyan Kulev]

Hi,

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 <ogi@fmi.uni-sofia.bg>, "\"Programmer\""
7D9F 66E6 68B7 A62B 0FCF  EB04 80BF 3A8C A252 9782

2003-06-11  Ognyan Kulev  <ogi@fmi.uni-sofia.bg>

        * 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  <ogi@fmi.uni-sofia.bg>

        * 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;
 }