[PATCH] Allow clients to check out special files with unknown type

Hi all,

Here's a patch to solve part of issue 2692. As described in that issue,
anyone with a Windows client (or Linux client on a VFAT drive, or some
other non-symlink-supporting fs) can check out symlinks and get the
internal representation.

That's great, and it means that symlink-incapable clients can still
update symlinks. But it also means that they can change the special
file type (the 'link' in the internal representation).

And if a special file with an unknown type is committed to the
repository (and we don't check to see whether it's valid), anyone else
attempting to checkout/update gets a fatal error. The only real
resolution is to delete the symlink remotely.

One common way to screw things up seems to be to access a wc over the
network (via CIFS or NFS, say) that you've previously checked out via a
symlink-capabable client. Assuming the fileserver is able to store
symlinks, this means that your Windows client will probably now see the
symlink as a regular file with the contents of the file it points to.
Committing from that wc (which now shows the symlink as modified) then
causes the badness described above.

So, the attached patch goes some way towards fixing this. It makes an
unknown special file type a non-fatal error, and handles it just the
same way as if it can't create a symlink - by creating a regular file
with the internal representation.

There's still a problem with 'status' afterwards, since it reports the
new regular file as obstructing what it thinks should be a symlink, but
this is the same problem that already occurs with symlinks in the
Linux/VFAT cases - and in any case it's a better situation than we have
now.

Issue 2692 sets out three things that we can do to fix this problem:

1. Make special files of unknown type work like symlinks on Windows.
2. Fix status not to assume that HAVE_SYMLINK means that all special
   files will be symlinks.
3. Possibly add some form of sanity checking to the client to make
   the problems above more difficult to cause.

The patch attached does #1 above. I'm not promising to work on #2 or
#3. Any comments on the approach or the patch?

[[[
Fix part of issue 2692: allow clients to checkout unknown special file
types as regular files with the svn:special property set (like symlinks
on Windows), instead of treating an unknown special file type as a fatal
error (which, since we don't validate the type on commit, allows one
client to DoS all the others).

* subversion/libsvn_subr/subst.c
  (create_special_file_from_stringbuf): Create a regular file containing
    the special file's internal representation if the special file type
    was unknown, re-using the code for creating 'symlinks' on Windows
    otherwise.

* subversion/tests/cmdline/special_tests.py
  (checkout_repo_with_unknown_special_type): New. Check that we can
    check out a repository containing a special file of unknown type.
  (test_list): Add checkout_repo_with_unknown_special_type.

* subversion/tests/cmdline/special_tests_data/bad-special-type.dump
  New. Dumpfile for checkout_repo_with_unknown_special_type.
]]]

Regards,
Malcolm

Index: subversion/libsvn_subr/subst.c
===================================================================
--- subversion/libsvn_subr/subst.c (revision 23397)
+++ subversion/libsvn_subr/subst.c (working copy)
@@ -1567,9 +1567,9 @@ static svn_error_t *
 create_special_file_from_stringbuf(svn_stringbuf_t *src, const char *dst,
                                    apr_pool_t *pool)
 {
- svn_error_t *err;
   char *identifier, *remainder;
   const char *dst_tmp;
+ svn_boolean_t create_using_internal_representation = FALSE;
 
   /* Separate off the identifier. The first space character delimits
      the identifier, after which any remaining characters are specific
@@ -1589,38 +1589,45 @@ create_special_file_from_stringbuf(svn_s
     {
       /* For symlinks, the type specific data is just a filesystem
          path that the symlink should reference. */
- err = svn_io_create_unique_link(&dst_tmp, dst, remainder,
- ".tmp", pool);
+ svn_error_t *err = svn_io_create_unique_link(&dst_tmp, dst, remainder,
+ ".tmp", pool);
+
+ /* If we had an error, check to see if it was because this type of
+ special device is not supported. If so, fall back to using the
+ internal representation. */
+ if (err)
+ {
+ if (err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
+ {
+ svn_error_clear(err);
+ create_using_internal_representation = TRUE;
+ }
+ else
+ return err;
+ }
     }
   else
     {
- /* We should return a valid error here. */
- return svn_error_createf(SVN_ERR_UNSUPPORTED_FEATURE, NULL,
- _("'%s' has unsupported special file type "
- "'%s'"), src->data, identifier);
+ /* Just create a normal file using the internal special-file
+ representation. We don't want a commit of an unknown special
+ type to DoS all the clients. */
+ create_using_internal_representation = TRUE;
     }
 
- /* If we had an error, check to see if it was because this type of
- special device is not supported. */
- if (err)
+ /* If nothing else worked, write out the internal representation to
+ a file that can be edited by the user. */
+ if (create_using_internal_representation)
     {
- if (err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
- {
- apr_file_t *dst_tmp_file;
- apr_size_t written;
+ apr_file_t *dst_tmp_file;
+ apr_size_t written;
 
- svn_error_clear(err);
 
- /* Fall back to just copying the text-base. */
- SVN_ERR(svn_io_open_unique_file2(&dst_tmp_file, &dst_tmp,
- dst, ".tmp", svn_io_file_del_none,
- pool));
- SVN_ERR(svn_io_file_write_full(dst_tmp_file, src->data, src->len,
- &written, pool));
- SVN_ERR(svn_io_file_close(dst_tmp_file, pool));
- }
- else
- return err;
+ SVN_ERR(svn_io_open_unique_file2(&dst_tmp_file, &dst_tmp,
+ dst, ".tmp", svn_io_file_del_none,
+ pool));
+ SVN_ERR(svn_io_file_write_full(dst_tmp_file, src->data, src->len,
+ &written, pool));
+ SVN_ERR(svn_io_file_close(dst_tmp_file, pool));
     }
 
   /* Do the atomic rename from our temporary location. */
Index: subversion/tests/cmdline/special_tests_data/bad-special-type.dump
===================================================================
--- subversion/tests/cmdline/special_tests_data/bad-special-type.dump (revision 0)
+++ subversion/tests/cmdline/special_tests_data/bad-special-type.dump (revision 0)
@@ -0,0 +1,47 @@
+SVN-fs-dump-format-version: 2
+
+UUID: 51ec0b24-bd07-11db-97f6-8dc527e3df93
+
+Revision-number: 0
+Prop-content-length: 56
+Content-length: 56
+
+K 8
+svn:date
+V 27
+2007-02-15T15:16:13.268177Z
+PROPS-END
+
+Revision-number: 1
+Prop-content-length: 143
+Content-length: 143
+
+K 7
+svn:log
+V 41
+Commit a special file with a unknown type
+K 10
+svn:author
+V 7
+malcolm
+K 8
+svn:date
+V 27
+2007-02-15T15:18:08.532835Z
+PROPS-END
+
+Node-path: special
+Node-kind: file
+Node-action: add
+Prop-content-length: 33
+Text-content-length: 11
+Text-content-md5: 12dfeb21c0626cc041b5de87de30c661
+Content-length: 44
+
+K 11
+svn:special
+V 1
+*
+PROPS-END
+gimble wabe
+
Index: subversion/tests/cmdline/special_tests.py
===================================================================
--- subversion/tests/cmdline/special_tests.py (revision 23397)
+++ subversion/tests/cmdline/special_tests.py (working copy)
@@ -531,6 +531,33 @@ def diff_symlink_to_dir(sbox):
   svntest.actions.run_and_verify_svn(None, expected_output, [], 'diff',
                                      link_path)
 
+# Issue 2692 (part of): Check that the client can check out a repository
+# that contains an unknown special file type.
+def checkout_repo_with_unknown_special_type(sbox):
+ "checkout repository with unknown special file type"
+
+ # Create virgin repos and working copy
+ svntest.main.safe_rmtree(sbox.repo_dir, 1)
+ svntest.main.create_repos(sbox.repo_dir)
+
+ # Load the dumpfile into the repos.
+ data_dir = os.path.join(os.path.dirname(sys.argv[0]),
+ 'special_tests_data')
+ dump_str = file(os.path.join(data_dir,
+ "bad-special-type.dump"), "rb").read()
+ svntest.actions.run_and_verify_load(sbox.repo_dir, dump_str)
+
+ expected_output = svntest.wc.State(sbox.wc_dir, {
+ 'special': Item(status='A '),
+ })
+ expected_wc = svntest.wc.State('', {
+ 'special' : Item(contents='gimble wabe'),
+ })
+ svntest.actions.run_and_verify_checkout(sbox.repo_url,
+ sbox.wc_dir,
+ expected_output,
+ expected_wc)
+
 
 ########################################################################
 # Run the tests
@@ -548,6 +575,7 @@ test_list = [ None,
               XFail(Skip(merge_file_into_symlink, (os.name != 'posix'))),
               checkout_repo_with_symlinks,
               XFail(Skip(diff_symlink_to_dir, (os.name != 'posix'))),
+ checkout_repo_with_unknown_special_type,
              ]
 
 if __name__ == '__main__':