cvs-1.11.1p1 LockDir and symlinks lossage and workaround

[Loic Dachary]

        Hi,

        There is a pending bug that shows under various conditions
when symlinks are used in the CVSROOT path. A simple test case is as
follows (there are maybe simpler test cases but this one always
reproduces the problem):

        - mkdir /tmp/bar
        - ln -s /tmp/bar /tmp/cvsroot
        - CVSROOT=/tmp/cvsroot cvs init
        - create file a/test1 in the repostiroy
        - arrange for /tmp/cvsroot to be accessed thru pserver
        - in CVSROOT/config say that you need a LockDir=/tmp/lock
        - mkdir /tmp/lock
        - cvs -d :pserver:host:/tmp/cvsroot rdiff  -D '2001/12/01' -D 
'2001/12/12' a/test1

        The later displays the following (useless) assert error:

cvs: lock.c:178: lock_name: Assertion `(__extension__ (__builtin_constant_p ( 
strlen (current_parsed_root->directory)) && ((__builtin_constant_p (repository) 
&& strlen (repository) < ((size_t) ( strlen (current_parsed_root->directory)))) 
|| (__builtin_constant_p ( current_parsed_root->directory) && strlen ( 
current_parsed_root->directory) < ((size_t) ( strlen 
(current_parsed_root->directory))))) ? __extension__ ({ size_t __s1_len, 
__s2_len; (__builtin_constant_p (repository) && __builtin_constant_p ( 
current_parsed_root->directory) && (__s1_len = strlen (repository), __s2_len = 
strlen ( current_parsed_root->directory), (!((size_t)(const void 
*)((repository) + 1) - (size_t)(const void *)(repository) == 1) || __s1_len >= 
4) && (!((size_t)(const void *)(( current_parsed_root->directory) + 1) - 
(size_t)(const void *)( current_parsed_root->directory) == 1) || __s2_len >= 
4)) ? memcmp ((__const char *) (repository), (__const char *) ( 
current_parsed_root->directory), (__s1_len < __s2_len ? __s1_len : __s2_len) + 
1) : (__builtin_constant_p (repository) && ((size_t)(const void *)((repository) 
+ 1) - (size_t)(const void *)(repository) == 1) && (__s1_len = strlen 
(repository), __s1_len < 4) ? (__builtin_constant_p ( 
current_parsed_root->directory) && ((size_t)(const void *)(( 
current_parsed_root->directory) + 1) - (size_t)(const void *)( 
current_parsed_root->directory) == 1) ? (__extension__ ({ register int __result 
= (((__const unsigned char *) (__const char *) (repository))[0] - ((__const 
unsigned char *) (__const char *)( current_parsed_root->directory))[0]); if ( 
__s1_len > 0 && __result == 0) { __result = (((__const unsigned char *) 
(__const char *) (repository))[1] - ((__const unsigned char *) (__const char *) 
( current_parsed_root->directory))[1]); if ( __s1_len > 1 && __result == 0) { 
__result = (((__const unsigned char *) (__const char *) (repository))[2] - 
((__const unsigned char *) (__const char *) ( 
current_parsed_root->directory))[2]); if ( __s1_len > 2 && __result == 0) 
__result = (((__const unsigned char *) (__const char *) (repository))[3] - 
((__const unsigned char *) (__const char *) ( 
current_parsed_root->directory))[3]); } } __result; })) : (__extension__ ({ 
__const unsigned char *__s2 = (__const unsigned char *) (__const char *) ( 
current_parsed_root->directory); register int __result = (((__const unsigned 
char *) (__const char *) (repository))[0] - __s2[0]); if ( __s1_len > 0 && 
__result == 0) { __result = (((__const unsigned char *) (__const char *) 
(repository))[1] - __s2[1]); if ( __s1_len > 1 && __result == 0) { __result = 
(((__const unsigned char *) (__const char *) (repository))[2] - __s2[2]); if ( 
__s1_len > 2 && __result == 0) __result = (((__const unsigned char *) (__const 
char *) (repository))[3] - __s2[3]); } } __result; }))) : (__builtin_constant_p 
( current_parsed_root->directory) && ((size_t)(const void *)(( 
current_parsed_root->directory) + 1) - (size_t)(const void *)( 
current_parsed_root->directory) == 1) && (__s2_len = strlen ( 
current_parsed_root->directory), __s2_len < 4) ? (__builtin_constant_p 
(repository) && ((size_t)(const void *)((repository) + 1) - (size_t)(const void 
*)(repository) == 1) ? (__extension__ ({ register int __result = (((__const 
unsigned char *) (__const char *) (repository))[0] - ((__const unsigned char *) 
(__const char *)( current_parsed_root->directory))[0]); if ( __s2_len > 0 && 
__result == 0) { __result = (((__const unsigned char *) (__const char *) 
(repository))[1] - ((__const unsigned char *) (__const char *) ( 
current_parsed_root->directory))[1]); if ( __s2_len > 1 && __result == 0) { 
__result = (((__const unsigned char *) (__const char *) (repository))[2] - 
((__const unsigned char *) (__const char *) ( 
current_parsed_root->directory))[2]); if ( __s2_len > 2 && __result == 0) 
__result = (((__const unsigned char *) (__const char *) (repository))[3] - 
((__const unsigned char *) (__const char *) ( 
current_parsed_root->directory))[3]); } } __result; })) : (__extension__ ({ 
__const unsigned char *__s1 = (__const unsigned char *) (__const char *) 
(repository); register int __result = __s1[0] - ((__const unsigned char *) 
(__const char *) ( current_parsed_root->directory))[0]; if ( __s2_len > 0 && 
__result == 0) { __result = (__s1[1] - ((__const unsigned char *) (__const char 
*) ( current_parsed_root->directory))[1]); if ( __s2_len > 1 && __result == 0) 
{ __result = (__s1[2] - ((__const unsigned char *) (__const char *) ( 
current_parsed_root->directory))[2]); if ( __s2_len > 2 && __result == 0) 
__result = (__s1[3] - ((__const unsigned char *) (__const char *) ( 
current_parsed_root->directory))[3]); } } __result; }))) : strcmp (repository, 
current_parsed_root->directory)))); }) : strncmp (repository, 
current_parsed_root->directory, strlen (current_parsed_root->directory)))) == 
0' failed.
cvs [server aborted]: received abort signal

        Tracing the error shows that "repository" = /tmp/bar/a where
"current_parsed_root->directory" = /tmp/cvsroot/a. This is because "repository"
was acquired by xgetwd (lib/xgetwd.c in the cvs sources) and has no way
to figure out that it got there thru a symlink.

        I guess the ultimate solution would be to avoid using xgetwd at
all to prevent that kind of lossage. However it was a bit too hard for
me to figure out and I prefered to arrange for all chdir to go to a function
that would remember the path each time it is called. This is a hack, here
it is:

diff -ru ../.old/cvs-1.11.1p1/lib/savecwd.c ./lib/savecwd.c
--- ../.old/cvs-1.11.1p1/lib/savecwd.c  Thu Apr 19 15:45:30 2001
+++ ./lib/savecwd.c     Mon Dec 17 11:35:24 2001
@@ -50,7 +50,7 @@
 save_cwd (cwd)
      struct saved_cwd *cwd;
 {
-  static int have_working_fchdir = 1;
+  static int have_working_fchdir = 0;
 
   cwd->desc = -1;
   cwd->name = NULL;
@@ -122,7 +122,7 @@
          fail = 1;
        }
     }
-  else if (chdir (cwd->name) < 0)
+  else if (xchdir (cwd->name) < 0)
     {
       error (0, errno, "%s", cwd->name);
       fail = 1;
diff -ru ../.old/cvs-1.11.1p1/lib/system.h ./lib/system.h
--- ../.old/cvs-1.11.1p1/lib/system.h   Thu Apr 19 15:45:30 2001
+++ ./lib/system.h      Mon Dec 17 11:37:10 2001
@@ -389,7 +389,8 @@
 #endif
 
 #ifndef CVS_CHDIR
-#define CVS_CHDIR chdir
+#define CVS_CHDIR xchdir
+extern int xchdir(const char *path);
 #endif
 
 #ifndef CVS_CREAT
diff -ru ../.old/cvs-1.11.1p1/lib/xgetwd.c ./lib/xgetwd.c
--- ../.old/cvs-1.11.1p1/lib/xgetwd.c   Thu Apr 19 15:29:03 2001
+++ ./lib/xgetwd.c      Mon Dec 17 11:53:16 2001
@@ -25,6 +25,8 @@
 extern int errno;
 #endif
 #include <sys/types.h>
+#include <assert.h>
+#include <strings.h>
 
 /* Amount by which to increase buffer size when allocating more space. */
 #define PATH_INCR 32
@@ -32,36 +34,97 @@
 char *xmalloc ();
 char *xrealloc ();
 
+/*
+ * Keep the current path in a buffer. This makes the work of xgetwd
+ * simpler AND prevent lossage when symbolic links are followed when
+ * descending the CVS repository. A typical command that always fails
+ * when a symbolic link is found somewhere in the path is 
+ * cvs rdiff  -D '2001/12/01' -D '2001/12/12' file.
+ * The implementation is a hack that will NOT work in the general
+ * case. But is copes with all forms of chdir that are in use
+ * in the sources. Namely, it will lose when using .. in the middle
+ * of the path or ./././ etc.
+ */
+
+static char __cwd[PATH_MAX + 1] = { '\0' };
+
+int xchdir(const char* path)
+{
+  int path_length = strlen(path);
+  int retval;
+
+  /* fprintf(stderr, "chdir(%d): %s\n", getpid(), path); */
+
+  if((retval = chdir(path)) != 0) {
+    return retval;
+  }
+
+  assert(path_length <= PATH_MAX);
+
+  if (path_length == 1 && path[0] == '.')
+    return 0;
+
+  if (path_length == 2 && path[0] == '.' && path[1] == '.') {
+    char* last_slash = 0;
+    int cwd_length = strlen(__cwd);
+    if (__cwd[0] == '/' && cwd_length > 1 && (last_slash = strrchr(__cwd + 1, 
'/')))
+      *last_slash = '\0';
+  } else {
+    int cwd_length;
+    if (path[0] == '/') {
+      strcpy(__cwd, path);
+    } else {
+      assert((path_length + strlen(__cwd) + 1) <= PATH_MAX);
+      strcat(__cwd, "/");
+      strcat(__cwd, path);
+    }
+
+    /* Strip trailing /. */
+    cwd_length = strlen(__cwd);
+    if(cwd_length > 2 && __cwd[cwd_length - 2] == '/' && __cwd[cwd_length - 1] 
== '.')
+      __cwd[cwd_length - 2] = '\0';
+  }
+
+  /* fprintf(stderr, "chdir(%d): %s -> %s\n", getpid(), path, __cwd); */
+
+  return retval;
+}
+
 /* Return the current directory, newly allocated, arbitrarily long.
    Return NULL and set errno on error. */
 
 char *
 xgetwd ()
 {
-  char *cwd;
-  char *ret;
-  unsigned path_max;
-
-  errno = 0;
-  path_max = (unsigned) PATH_MAX;
-  path_max += 2;               /* The getcwd docs say to do this. */
-
-  cwd = xmalloc (path_max);
-
-  errno = 0;
-  while ((ret = getcwd (cwd, path_max)) == NULL && errno == ERANGE)
-    {
-      path_max += PATH_INCR;
-      cwd = xrealloc (cwd, path_max);
-      errno = 0;
-    }
-
-  if (ret == NULL)
-    {
-      int save_errno = errno;
-      free (cwd);
-      errno = save_errno;
-      return NULL;
-    }
-  return cwd;
+  if (__cwd[0] != '\0') {
+    /* fprintf(stderr, "xgetwd(%d): %s\n", getpid(), __cwd); */
+    return strdup(__cwd);
+  } else {
+    char *cwd;
+    char *ret;
+    unsigned path_max;
+
+    errno = 0;
+    path_max = (unsigned) PATH_MAX;
+    path_max += 2;             /* The getcwd docs say to do this. */
+
+    cwd = xmalloc (path_max);
+
+    errno = 0;
+    while ((ret = getcwd (cwd, path_max)) == NULL && errno == ERANGE)
+      {
+       path_max += PATH_INCR;
+       cwd = xrealloc (cwd, path_max);
+       errno = 0;
+      }
+
+    if (ret == NULL)
+      {
+       int save_errno = errno;
+       free (cwd);
+       errno = save_errno;
+       return NULL;
+      }
+    return cwd;
+  }
 }


        Should you want to check that it works properly, it is running
on cvs -d :pserver:anoncvs@subversions.gnu.org:/cvsroot/gcc co gcc
(the corresponding cvsroot has symlinks). If you ever find something
wrong with this patch, I'd appreciate if you could send a note to
savannah-hackers@gnu.org so that we can fix the problem.

        Cheers,

-- 
Loic   Dachary         http://www.dachary.org/  loic@dachary.org
12 bd  Magenta         http://www.senga.org/      loic@senga.org
75010    Paris         T: 33 1 42 45 07 97          loic@gnu.org
        GPG Public Key: http://www.dachary.org/loic/gpg.txt