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

[Derek Robert Price]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Mark D. Baushke wrote:

|Derek Robert Price <derek@ximbiot.com> writes:
|
|>Mark D. Baushke wrote:
|>
|>| For 1.12.x, it might be reasonable to use the GNULIB version of
|>|
|>|canonicalize_file_name() by adding a lib/canonicalize.c file that
|>|gets built if the function does not exist on the target platform.
|>
|>Is there a GNULIB version?
|
|
|Yes. I see stdlib/canonicalize.c in glibc and I see the (more portable?)
|coreutils project has the files:


There is an actual GNULIB project, as opposed to glibc or coreutils.
You can find it at <http://www.gnu.org/software/gnulib/>.  There is much
interchange among GNULIB, glibc, coreutils, and I think several other
projects, with GNULIB's files often originating in those locations, and
I believe the various teams are trying to move the "canonical" version
of any function that exists in multiple locations into GNULIB when
possible (meaning glibc and coreutils would get their versions from
GNULIB rather than vice versa.  One of the major differences between the
projects is that the GNULIB versions of the sources are usually released
under the GPL while at least glibc releases (often identical code with
the permission of the FSF) under the LGPL.  We might be able to get
similar license dispensation from the FSF for any file we needed from
glibc, but it might be easiest to go through the GNULIB folks if we
really need a file from glibc.

|    m4/canonicalize.m4
|    lib/canonicalize.c
|    lib/canonicalize.h
|
|Taking a quick look inside of the m4 file I see the following:


. . .

|I am not sure where you have been getting the GNULIB functions for the
|lib and m4 files for our development version of cvs, so if this is the
|wrong place, please let me know.


I've been maintaining ccvs/srclist.txt on 1.12.x with the source of
various files we use in CVS and notes on project URLs, CVS repositories,
and whether they contain local modifications.

I agree that the version of canonicalize in coreutils looks pretty
complete if you want to import it.  Please maintain srclist.txt if you do.

|FWIW: I like canonicalize_file_name() better than the POSIX realpath()
|function because the POSIX function is unable to deal with an unlimited
|maximal path size. Hmmm.... It appears that the coreutils implementation
|may use a sytem version of the realpath() function if available.


Is this an issue?  It looks like canonicalize.c uses MAX_PATH, which I
thought was a limitation on systems regardless of their supplying of
realpath().

canonicalize.c appears to use MAX_PATH as a limit whether it uses
realpath() or not.

|>I did just notice an interesting module in GNULIB named "same" that uses
|>stat to compare two paths and verify that they point to the same inode,
|>but that could be overkill in this case.  We couldn't use the "same"
|>module directly regardless since it requires that the last component of
|>the path (determined via basename) be the same string for some reason
|>rather than using the inode there too.
|
|
|So, appending '/.' to the pathname would be needed? Kind of ugly, but
|I suppose it might work.


Or we could hack the "same" module to only compare inodes when we
imported it.

|In any case, the lock_name() wants to take a full pathname to a
|directory in the repository and determine what part of that name
|is the same as the cvsroot and what is the module pathname so that
|it can create a lock in the LockDir specified directory correctly.
|
|So, I guess it would need to lop off successive elements from the
|repository until it finds one with the same inode as the
|current_parsed_root->directory ... that would be an approach that should
|work and might even get around problems with the directory permissions
|in the intervening directories making it difficult to find the path that
|is really wanted. However, it seems like cvs is already relying on the
|ability to have xgetwd() to return a full pathname without any problems.


Well, paths in error messages get munged.

|>This sounds like a bug in that code, if it even exists at all anymore
|>in 1.12.x. You didn't mention which version you found this problem in.
|>As for 1.11.x, symlinks are pretty darn broke and I wouldn't bother
|>fixing one single bug there anyhow.
|
|
|It would seem to be a matter of motivation... I found it in version
|1.11.6 and opened issue #142 with it before I managed to find issue #33.
|My problem is actually a subest of #33 in that.
|
|>I had to overhaul quite a bit of code to get symlinks to work, which
|>is why I only checked the fix into 1.12.x.
|
|
|I understand.
|
|In my case, I am doing work for a company that wants me to solve this
|problem in the 1.11.x code base. While I could just give them a local
|patch, it seems reasonable to me that the work should get put into
|the cvshome source base so that a 1.11.7 version will not have this
|particular problem. Yes, other problems may exist with symlinks, but
|this is the one that is stopping them right now.
|
|I verified that the problem was still present in 1.11.6.1 (cvs version).
|
|I have verified that my patch to lock.c and root.c fixes the problem and
|I have provided a test case that shows that the problem is fixed (to the
|extent that the assert no longer fails and at least somewhat reasonable
|output is provided for the r* commands).


I don't think I actually want to try and stop you, though I'm a bit
leery of starting to apply band-aids to 1.11.x when the problem is fixed
correctly in 1.12.x.  I'm more afraid of the next fix and the fix after
that.   It seems only a matter of time until the next symlink problem is
discovered - there were quite a few, but perhaps I should lay off until
those fixes are suggested.  :)  We could discuss the feasability of
backporting my 1.12.x fix.  Either way I'm a bit afraid of destabilizing
1.11.x.

| It is also interesting that the RCS File: is 'more correct' in the
|
|1.12.x version as it uses the symlink name rather than the canonical
|name as a prefix in the path to the RCS,v filename.


I'm not sure I agree that this is more correct, actually.  cvshome.org,
in particular, allows access to its CVS repository via two or three
separate symlinks for historical reasons, but in the case of a single
symlink that is the only --allowed-root, I will agree that it is more
correct.

|You may close issue #33 as fixed in the 1.12 development version, but it
|is still broken in the 'stable' version of cvs and it is difficult to
|get folks to move off a 'stable' version of the release for production
|use...


I do understand this, but have you considered maintaining a backport of
the 1.12.x symlink fix patch for these people?  You would hopefully be
able to drop it after the 1.13 release.  The fix would be more complete
- - they wouldn't have to worry about the other symlink bugs you already
know are waiting for their encounter.

|>I'm not sure whether it would be useful to commit your test case.  I
|>hacked cvsroot at the beginning of sanity.sh to be a symlink and ran the
|>entire suite when I first made symlinks work in 1.12.x and am making
|>every test run with a symlinked root in addition to a standard one as
|>part of the new autotest suite.
|
|
|Excellent.


Actually, running sanity.sh with a -l argument appears to do this on
1.12.x.  I didn't recall leaving that hack there.

Anyhow, does your lockdir test add tests that weren't tested _without_ a
symlink before?  If so, I would appreciate those tests being added to
1.12.x.

| I will note that xresolvepath() has an 'assert ( isdir (path) );' in it.
|
|This is make the function less useful for the purpose I need in the
|1.11.x version of parse_cvsroot() and local_cvsroot() the 'cvs init'
|command passes in a directory that does not yet exist, so I would need
|to duplicate the isdir (path) call before calling xresolvepath() to
|avoid the assert, when getting a NULL return in that case is acceptable.


That assert was just to stop developers from assuming xresolvepath had
error checking in it.  It should be able to be removed without affecting
any of the functions that are currently using xresolvepath, though error
checking may have to be added.  Does canonicalize_file_name resolve
paths that don't exist yet?

|I am all for centralizing code that need not be duplicated and
|maintained in multiple filesubr.c versions.


I like the idea of importing canonicalize_file_name from coreutils.

If you do end up checking this fix into 1.11.x, please make sure to move
the last-merge tag so noone tries to merge them into 1.12.x.  Except
possibly for your test cases, which might be useful to merge there.

I haven't closed issue #33 yet.  I want to wait until we decide whether
this fix is valid for 1.11.x.

Derek

- --
~                *8^)

Email: derek@ximbiot.com

Get CVS support at <http://ximbiot.com>!
- --
That liberty [is pure] which is to go to all, and not to the few or the
rich alone.
           - Thomas Jefferson to H. Gates, 1798.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.0.7 (GNU/Linux)
Comment: Using GnuPG with Netscape - http://enigmail.mozdev.org

iD8DBQE/cZi5LD1OTBfyMaQRAm1aAJ0TjIiJ3jCGmg4OHRVWRclWRWDXUACglInf
0s3zE2XOTq8uBpUp8b+O9H8=
=+hxX
-----END PGP SIGNATURE-----