Re: cvs has problems using LockDir when CVSROOT holds a symlink

[Mark D. Baushke]

Hi Larry,

Larry Jones <lawrence.jones@eds.com> writes:

> Mark D. Baushke writes:
> > 
> > I will rework my patch to follow Derek's suggested advice.
> > 
> > I will probably use realpath() to create the canonical path rather than
> > either xwcd() or xreadlink().
> 
> Please don't -- realpath() does not exist on all systems.  Also, the
> most common implementation just calls getcwd() to do the real work
> anyway, so we might as well just use xgetwd().

Yeah, I noticed that realpath() was not present on all systems...
especially those that do not support symbolic links.

> You can also simplify the code in lock_name():

Yup. I did almost exactly what you suggest although I added one more
assert:

     assert (current_parsed_root->dirpath != NULL);

before I tried to do the strlen of it.

The 'make check' step is still running, but here is the patch in its
current form.

        Thanks,
        -- Mark

Index: src/ChangeLog
===================================================================
RCS file: /cvs/ccvs/src/ChangeLog,v
retrieving revision 1.2336.2.99
diff -u -p -r1.2336.2.99 ChangeLog
--- src/ChangeLog       12 Sep 2003 18:50:36 -0000      1.2336.2.99
+++ src/ChangeLog       23 Sep 2003 18:38:47 -0000
@@ -1,3 +1,18 @@
+2003-09-23  Mark D. Baushke  <mdb@gnu.org>
+
+       * root.h (cvsroot_t): Add expanded_directory to the structure.
+
+       * root.c (expand_directory): Add new function to canonicalize a
+       cvsroot directory.
+       (parsed_cvsroot, local_cvsroot): Use it.
+
+       * lock.c (lock_name): Work around possible symbolic links in the
+       current_parsed_root->directory when using LockDir by also checking
+       the new current_parsed_root->expanded_directory.
+
+       * sanity.sh (Server): New test for LockDir config item with and
+       without CVSROOT using a symbolic link directory.
+
 2003-09-12  Derek Price  <derek@ximbiot.com>
 
        * sanity.sh (mkmodules): Correct comments.
Index: src/lock.c
===================================================================
RCS file: /cvs/ccvs/src/lock.c,v
retrieving revision 1.59.4.2
diff -u -p -r1.59.4.2 lock.c
--- src/lock.c  27 Jun 2003 19:12:03 -0000      1.59.4.2
+++ src/lock.c  23 Sep 2003 18:38:47 -0000
@@ -179,16 +179,24 @@ lock_name (repository, name)
     {
        struct stat sb;
        mode_t new_mode = 0;
+       size_t parsed_root_len;
 
        /* The interesting part of the repository is the part relative
           to CVSROOT.  */
        assert (current_parsed_root != NULL);
        assert (current_parsed_root->directory != NULL);
-       assert (strncmp (repository, current_parsed_root->directory,
-                        strlen (current_parsed_root->directory)) == 0);
-       short_repos = repository + strlen (current_parsed_root->directory) + 1;
+       assert (current_parsed_root->dirpath != NULL);
+       parsed_root_len = strlen (current_parsed_root->dirpath);
+       if (strncmp (repository, current_parsed_root->dirpath,
+                    parsed_root_len) != 0)
+       {
+           parsed_root_len = strlen (current_parsed_root->directory);
+           assert (strncmp (repository, current_parsed_root->directory,
+                            parsed_root_len) == 0);
+       }
+       short_repos = repository + parsed_root_len + 1;
 
-       if (strcmp (repository, current_parsed_root->directory) == 0)
+       if (short_repos[-1] == '\0')
            short_repos = ".";
        else
            assert (short_repos[-1] == '/');
Index: src/root.c
===================================================================
RCS file: /cvs/ccvs/src/root.c,v
retrieving revision 1.60.2.4
diff -u -p -r1.60.2.4 root.c
--- src/root.c  28 Jun 2003 03:28:29 -0000      1.60.2.4
+++ src/root.c  23 Sep 2003 18:38:47 -0000
@@ -12,6 +12,7 @@
 
 #include "cvs.h"
 #include "getline.h"
+#include "savecwd.h"
 
 /* Printable names for things in the current_parsed_root->method enum variable.
    Watch out if the enum is changed in cvs.h! */
@@ -296,6 +297,7 @@ new_cvsroot_t ()
     newroot->hostname = NULL;
     newroot->port = 0;
     newroot->directory = NULL;
+    newroot->dirpath = NULL;
 #ifdef CLIENT_SUPPORT
     newroot->isremote = 0;
 #endif /* CLIENT_SUPPORT */
@@ -324,12 +326,49 @@ free_cvsroot_t (root)
        free (root->hostname);
     if (root->directory != NULL)
        free (root->directory);
+    if (root->dirpath != NULL)
+       free (root->dirpath);
     free (root);
 }
 
 
 
 /*
+ * Return the canonicalized absolute pathname, if one exists.
+ * Otherwise, return the path itself (this helps in cases where the
+ * repository has not yet been created).
+ */
+static char *
+expand_directory (path)
+    char *path;
+{
+    struct saved_cwd cwd;
+    char *normalized_path = NULL;
+
+    if (path == NULL)
+       return path;
+
+    if ( isdir (path) )
+    {
+        /* Try to normalize using xgetwd(). */
+        if (save_cwd (&cwd))
+            error (1, 0, "can't save cwd");
+        if (chdir (path) < 0)
+            error (1, errno, "can't change to %s", path);
+        normalized_path = xgetwd();
+        if (restore_cwd (&cwd, 0))
+            error (1, errno, "can't restore cwd");
+        free_cwd (&cwd);
+    }
+
+    if (normalized_path == NULL)
+       normalized_path = xstrdup (path);
+
+    return normalized_path;
+}
+
+
+/*
  * Parse a CVSROOT string to allocate and return a new cvsroot_t structure.
  * Valid specifications are:
  *
@@ -665,6 +704,9 @@ parse_cvsroot (root_in)
        goto error_exit;
     }
     
+    if ( newroot->dirpath == NULL )
+        newroot->dirpath = expand_directory (newroot->directory);
+
     /* Hooray!  We finally parsed it! */
     free (cvsroot_save);
     return newroot;
@@ -744,6 +786,7 @@ local_cvsroot (dir)
      * and, otherwise, we want no trailing slashes.
      */
     Sanitize_Repository_Name( newroot->directory );
+    newroot->dirpath = expand_directory (newroot->directory);
     return newroot;
 }
 
Index: src/root.h
===================================================================
RCS file: /cvs/ccvs/src/root.h,v
retrieving revision 1.2
diff -u -p -r1.2 root.h
--- src/root.h  21 Jan 2003 17:59:40 -0000      1.2
+++ src/root.h  23 Sep 2003 18:38:47 -0000
@@ -31,6 +31,7 @@ typedef struct cvsroot_s {
     char *hostname;            /* the hostname or NULL if method == local */
     int port;                  /* the port or zero if method == local */
     char *directory;           /* the directory name */
+    char *dirpath;              /* the normalized CVSROOT path */
 #ifdef CLIENT_SUPPORT
     unsigned char isremote;    /* nonzero if we are doing remote access */
 #endif /* CLIENT_SUPPORT */
Index: src/sanity.sh
===================================================================
RCS file: /cvs/ccvs/src/sanity.sh,v
retrieving revision 1.752.2.39
diff -u -p -r1.752.2.39 sanity.sh
--- src/sanity.sh       12 Sep 2003 18:50:36 -0000      1.752.2.39
+++ src/sanity.sh       23 Sep 2003 18:38:47 -0000
@@ -747,7 +747,7 @@ if test x"$*" = x; then
        tests="${tests} serverpatch log log2 logopt ann ann-id"
        # Repository Storage (RCS file format, CVS lock files, creating
        # a repository without "cvs init", &c).
-       tests="${tests} crerepos rcs rcs2 rcs3 lockfiles backuprecover"
+       tests="${tests} crerepos rcs rcs2 rcs3 lockfiles lockdir backuprecover"
        # More history browsing, &c.
        tests="${tests} history"
        tests="${tests} big modes modes2 modes3 stamps"
@@ -18452,6 +18452,99 @@ ${PROG} commit: Rebuilding administrativ
          rm -r ${TESTDIR}/locks
          rm -r 1 2
          rm -rf ${CVSROOT_DIRNAME}/first-dir
+         ;;
+
+       lockdir)
+         # Tests that the full range of commands does the right
+         # thing with regard to LockDir directives.
+         mkdir lockdir; cd lockdir
+
+         dotest lockdir-1 "${testcvs} -Q co CVSROOT" ""
+         cd CVSROOT
+         echo "LockDir=${TESTDIR}/locks" >config
+         dotest lockdir-2 "${testcvs} -q ci -m config-it" \
+"Checking in config;
+${CVSROOT_DIRNAME}/CVSROOT/config,v  <--  config
+new revision: 1\.[0-9]*; previous revision: 1\.[0-9]*
+done
+${PROG} commit: Rebuilding administrative file database"
+         dotest_fail lockdir-3 "${testcvs} rlog -r1.1 CVSROOT/config" \
+"${PROG} .rlog aborted.: cannot stat ${TESTDIR}/locks: No such file or 
directory
+${PROG} .rlog aborted.: cannot stat ${TESTDIR}/locks: No such file or 
directory"
+         dotest lockdir-4 "mkdir ${TESTDIR}/locks" ""
+         dotest lockdir-5 "${testcvs} rlog -r1.1 CVSROOT/config" \
+"
+RCS file: ${CVSROOT_DIRNAME}/CVSROOT/config,v
+head: 1\.[0-9]*
+branch:
+locks: strict
+access list:
+symbolic names:
+keyword substitution: kv
+total revisions: [0-9]*;       selected revisions: 1
+description:
+----------------------------
+revision 1.1
+date: [0-9/]* [0-9:]*;  author: $username;  state: Exp;
+.*
+============================================================================="
+
+         dotest lockdir-6 "ln -s ${CVSROOT_DIRNAME} ${TESTDIR}/symlink" ""
+          if $remote; then
+           dotest lockdir-7r \
+"${testcvs} -d :fork:${TESTDIR}/symlink rlog -r1.1 CVSROOT/config" \
+"
+RCS file: ${CVSROOT_DIRNAME}/CVSROOT/config,v
+head: 1\.[0-9]*
+branch:
+locks: strict
+access list:
+symbolic names:
+keyword substitution: kv
+total revisions: [0-9]*;       selected revisions: 1
+description:
+----------------------------
+revision 1.1
+date: [0-9/]* [0-9:]*;  author: $username;  state: Exp;
+.*
+============================================================================="
+          else
+           dotest lockdir-7 \
+"${testcvs} -d ${TESTDIR}/symlink rlog -r1.1 CVSROOT/config" \
+"
+RCS file: ${CVSROOT_DIRNAME}/CVSROOT/config,v
+head: 1\.[0-9]*
+branch:
+locks: strict
+access list:
+symbolic names:
+keyword substitution: kv
+total revisions: [0-9]*;       selected revisions: 1
+description:
+----------------------------
+revision 1.1
+date: [0-9/]* [0-9:]*;  author: $username;  state: Exp;
+.*
+============================================================================="
+          fi
+       
+         echo "# nobody here but us comments" >config
+         dotest lockdir-cleanup-1 "${testcvs} -q ci -m config-it" \
+"Checking in config;
+${CVSROOT_DIRNAME}/CVSROOT/config,v  <--  config
+new revision: 1\.[0-9]*; previous revision: 1\.[0-9]*
+done
+${PROG} commit: Rebuilding administrative file database"
+
+         cd ../..
+
+         if $keep; then
+           echo Keeping ${TESTDIR} and exiting due to --keep
+           exit 0
+         fi
+
+         rm -fr lockdir ${TESTDIR}/locks         
+         rm -f ${TESTDIR}/symlink
          ;;
 
        backuprecover)