RE: svn commit: r1622937 -/subversion/trunk/subversion/libsvn_fs_fs/low_level.c

[Bert Huijben]

The dirent is canonical test is not the right test for fspaths. Dirents are 
platform specific, while fspaths aren't. E.g. this breaks fspaths containing 
':' on Windows.

I think there should be a valid fspath API for this use case, and if not we 
should add it.

Bert

-----Original Message-----
From: "stef...@apache.org" <stef...@apache.org>
Sent: ‎06/‎09/‎2014 23:16
To: "comm...@subversion.apache.org" <comm...@subversion.apache.org>
Subject: svn commit: r1622937 
-/subversion/trunk/subversion/libsvn_fs_fs/low_level.c

Author: stefan2
Date: Sat Sep  6 21:15:40 2014
New Revision: 1622937

URL: http://svn.apache.org/r1622937
Log:
Harden FSFS's low-level path parsers against data corruption.

* subversion/libsvn_fs_fs/low_level.c
  (read_change
   svn_fs_fs__read_noderev): All paths must be canonical and start with '/'

Modified:
    subversion/trunk/subversion/libsvn_fs_fs/low_level.c

Modified: subversion/trunk/subversion/libsvn_fs_fs/low_level.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_fs_fs/low_level.c?rev=1622937&r1=1622936&r2=1622937&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_fs_fs/low_level.c (original)
+++ subversion/trunk/subversion/libsvn_fs_fs/low_level.c Sat Sep  6 21:15:40 
2014
@@ -21,6 +21,7 @@
  */
 
 #include "svn_private_config.h"
+#include "svn_dirent_uri.h"
 #include "svn_hash.h"
 #include "svn_pools.h"
 #include "svn_sorts.h"
@@ -376,8 +377,12 @@ read_change(change_t **change_p,
                               _("Invalid mergeinfo-mod flag in rev-file"));
         }
     }
-  
+
   /* Get the changed path. */
+  if (*last_str != '/' || !svn_dirent_is_canonical(last_str, scratch_pool))
+    return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
+                            _("Invalid path in changes line"));
+
   change->path.len = strlen(last_str);
   change->path.data = apr_pstrdup(result_pool, last_str);
 
@@ -394,9 +399,10 @@ read_change(change_t **change_p,
       last_str = line->data;
       SVN_ERR(parse_revnum(&info->copyfrom_rev, (const char **)&last_str));
 
-      if (! last_str)
+      if (   *last_str != '/'
+          || !svn_dirent_is_canonical(last_str, scratch_pool))
         return svn_error_create(SVN_ERR_FS_CORRUPT, NULL,
-                                _("Invalid changes line in rev-file"));
+                                _("Invalid copy-from path in changes line"));
 
       info->copyfrom_path = apr_pstrdup(result_pool, last_str);
     }
@@ -869,6 +875,11 @@ svn_fs_fs__read_noderev(node_revision_t 
     }
   else
     {
+      if (*value != '/' || !svn_dirent_is_canonical(value, scratch_pool))
+        return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
+                            _("Non-canonical cpath field in node-rev '%s'"),
+                            noderev_id);
+
       noderev->created_path = apr_pstrdup(result_pool, value);
     }
 
@@ -888,7 +899,7 @@ svn_fs_fs__read_noderev(node_revision_t 
     {
       SVN_ERR(parse_revnum(&noderev->copyroot_rev, (const char **)&value));
 
-      if (*value == '\0')
+      if (*value != '/' || !svn_dirent_is_canonical(value, scratch_pool))
         return svn_error_createf(SVN_ERR_FS_CORRUPT, NULL,
                                  _("Malformed copyroot line in node-rev '%s'"),
                                  noderev_id);