CVS lockdir bug with repository symlinks

[Ambalu, Robert]

There is a bug in cvs when using a separate lock directory and the
repository has symlinked files within it.  If there are symlinks in the
repository, the locks will get placed at the location of the symlink rather
than the location of the actual file (which leads to inconsistent locking).
There is another bug that is currently in the released version of CVS, also
associated with having a separate lock directory.  If there's a separate
lock directory, when looking for reader locks (in readers_exist) it doesn't
check to see if there's a separate lock dir or not, so it always checks for
locks in the repository itself.
Included are the diffs for the bug fix.  The symlink bugfix has been in use
at Goldman Sachs for the past 6 months now with no problems (our repository
is used by hundreds of developers every day).  The reader lock bug has been
in use for about a month and also has shown no problems.

Unfortunately the diffs are against the 1.11.2 release of CVS.  All diffs
are contained in lock.c, so hopefully it will not be hard to merge.  Another
issue to point out is that I have not tried compiling this on Windows (as
there were problems with the 1.11.2 release when building on Windows).  The
only incompatibility I can think of is the use of readlink(), which expands
symlinks and relative directories given a pathname.  But seeing as Windows
doesn't support symlinks #defining a readlink() that returns its input
should suffice.
Please let me know if/when the bug would make it into the main branch, we
would like to have this fix incorporated into your releases so we don't have
to patch every new release.
Thank You
- Rob Ambalu



Symlinks in repository when using separate lock dir bugfix diffs (against
CVS version 1.11.2)

160a161,164
>       /*full path in actual repository, less relative portions and 
>         symbolic links*/
>       char real_path[PATH_MAX]; 
> 
179d182
<       short_repos = repository + strlen (current_parsed_root->directory) +
1;
181c184,229
<       if (strcmp (repository, current_parsed_root->directory) == 0)
---
>       /*in order to avoid problems with symlinks in the repository, which
would 
>         lead to putting locks in the wrong directories, expand the given
path 
>         to give the REAL path to the directory we are accessing*/
>       if(realpath(repository, real_path)==NULL)
>       {
>               error(1, errno, "Error evaluating real path for %s",
repository);
>       }
> 
>       /*if realpath != original given path, then some symlink or possibly 
>         relative dirs were expanded, run extra checks on expanded path,
such 
>         as making sure it exists and making sure it points to somewhere
within 
>         the repository*/
>       if(strcmp(real_path, repository))
>       {
>               
>               /*check if evaluated path actually exists, this may be
overkill 
>                 but doesn't hurt to check*/
>               if(access(real_path, F_OK)) 
>               {
>                       error(1, 0, "path %s evaluated to non-existant
location %s", 
>                                 repository, real_path);
>               }
>               
>               /*check to make sure evaluated path still remains in the
repository, 
>                 simply check the path against the cvs root dir*/
>               if(strncmp (real_path, current_parsed_root->directory, 
>
strlen(current_parsed_root->directory)))
>               {
>                       if(islink(repository))
>                       {
>                               error(1, 0, "Error: path %s (symlink to %s)
leads out of repository", 
>                                         repository, real_path);
>                       }
>                       else
>                               error(1, 0, "Error: path %s leads out of
repository", 
>                                         repository);
>               }
>       }
>       else
>       {
>               strcpy(real_path, repository);
>       }
>       
>       short_repos = real_path + strlen(current_parsed_root->directory) +
1;
> 
>       if (strcmp (real_path, current_parsed_root->directory) == 0)
279c327,328
<     return retval;
---
> 
>       return retval;


Reader locks bugfix (these diffs are against lock.c in CVS version 1.11.2
with the previous diff already applied):

687a688
>       char *real_repository_path;
693,694c694,699
<     if ((dirp = CVS_OPENDIR (repository)) == NULL)
<       error (1, 0, "cannot open directory %s", repository);
---
>       /*use lock_name to give use the real path to look in, which is
>         dependent on whether a separate lockdir is in use or not*/
>       real_repository_path = lock_name (repository, "");
> 
>     if ((dirp = CVS_OPENDIR (real_repository_path)) == NULL)
>       error (1, 0, "cannot open directory %s", real_repository_path);
706,707c711,712
<           line = xmalloc (strlen (repository) + strlen (dp->d_name) + 5);
<           (void) sprintf (line, "%s/%s", repository, dp->d_name);
---
>           line = xmalloc (strlen (real_repository_path) + strlen
(dp->d_name) + 5);
>           (void) sprintf (line, "%s/%s", real_repository_path,
dp->d_name);
742c747
<       error (0, errno, "error reading directory %s", repository);
---
>       error (0, errno, "error reading directory %s",
real_repository_path);