Re: svn commit: r1877712 - in /subversion/trunk/subversion/tests/cmdline: entries-dump.c svntest/main.py

[Daniel Shahaf]

futat...@apache.org wrote on Thu, 14 May 2020 01:46 -:
> Log:
> Use safe bytes literals when set string values in working copy entries. 
> 

If someone reads this log message in a year, I don't think it'd be clear
to them what the problem being fixed was.  I think it would be useful to
be more specific about the circumstances; for example:

entries-dump: Escape string-typed attribute values when serializing
them as Python string literals.

Before this commit, a filesystem node named «foo\bar» (a single,
7-character path component) would cause «e.name = 'foo\bar'» to be
emitted.  The unescaped backslash would manifest as a test failure or
a SyntaxError, depending on the following characters.

This was triggered by svnadmin_tests 42 verify_metadata_only under
Python 3 on Windows.

The reference to "svnadmin_tests 42 verify_metadata_only" is only
a placeholder, to be replaced by the correct reference.

I specifically used the term "filesystem node" to clarify that «foo\bar»
was to be taken as an svn_fspath__* path (where backslash has no
special meaning), as opposed to, say, an svn_dirent_* path (where
backslash is a delimiter on Windows).

> * subversion/tests/cmdline/entries-dump.c
>   (print_prefix): New function.
>   (str_value):
>   - Add argument to specify pool.
>   - Print human readable value of "value" as is in comment, then set it
> as str value by using hex escaped bytes literal.
>   (entries_dump): Add pool argument to str_value() calls.
>   (main):
>   - Print "Entry" class definition as prefix before entry_dump() or 
> tree_dump() 
>   - Style fix on if statement (using blocks). 
>   (): Add include files for assert() and svn_xml_escape_attr_cstring()
> * subversion/tests/cmdline/svntest/main.py
>   (run_entiresdump, run_entriesdump_tree): Move definition of "Entry" class
>   into generated code by entries-dump execution. 
> 
> Review By: danielsh

For future reference, the "${Verb}ed by" phrases in log messages are
case-sensitive:

tools/dev/contribulyze.py:521:field_re = re.compile(
tools/dev/contribulyze.py:522:   
'^(Patch|Review(ed)?|Suggested|Found|Inspired|Tested|Reported) by:'
tools/dev/contribulyze.py:523:   '\s*\S.*$')
⋮
tools/dev/contribulyze.py:566:m = field_re.match(line)
⋮
tools/dev/contribulyze.py:604:  m = field_re.match(line)

This doesn't matter in this case, of course, since the person being
credited is already a full committer; however, when crediting people who
aren't already full committers, the correct case form should be used so
the regexp matches and the contribulyzer pages are updated.  (Unless we
change the script)

Cheers,

Daniel

svn commit: r1877712 - in /subversion/trunk/subversion/tests/cmdline: entries-dump.c svntest/main.py

[futatuki]

Author: futatuki
Date: Thu May 14 01:46:35 2020
New Revision: 1877712

URL: http://svn.apache.org/viewvc?rev=1877712&view=rev
Log:
Use safe bytes literals when set string values in working copy entries. 

* subversion/tests/cmdline/entries-dump.c
  (print_prefix): New function.
  (str_value):
  - Add argument to specify pool.
  - Print human readable value of "value" as is in comment, then set it
as str value by using hex escaped bytes literal.
  (entries_dump): Add pool argument to str_value() calls.
  (main):
  - Print "Entry" class definition as prefix before entry_dump() or tree_dump() 
  - Style fix on if statement (using blocks). 
  (): Add include files for assert() and svn_xml_escape_attr_cstring()
* subversion/tests/cmdline/svntest/main.py
  (run_entiresdump, run_entriesdump_tree): Move definition of "Entry" class
  into generated code by entries-dump execution. 

Review By: danielsh

Modified:
subversion/trunk/subversion/tests/cmdline/entries-dump.c
subversion/trunk/subversion/tests/cmdline/svntest/main.py

Modified: subversion/trunk/subversion/tests/cmdline/entries-dump.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/entries-dump.c?rev=1877712&r1=1877711&r2=1877712&view=diff
==
--- subversion/trunk/subversion/tests/cmdline/entries-dump.c (original)
+++ subversion/trunk/subversion/tests/cmdline/entries-dump.c Thu May 14 
01:46:35 2020
@@ -23,6 +23,7 @@
 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -34,6 +35,7 @@
 #include "svn_pools.h"
 #include "svn_wc.h"
 #include "svn_dirent_uri.h"
+#include "svn_xml.h"
 
 #include "private/svn_wc_private.h"
 
@@ -41,12 +43,45 @@
 #include "../../libsvn_wc/lock.h"
 
 static void
-str_value(const char *name, const char *value)
+print_prefix(void)
+{
+  puts("class Entry(object):\n"
+   "  \"\"\"An Entry object represents one node's entry in a pre-1.6"
+   " .svn/entries file.\n\n"
+   "Similar to #svn_wc_entry_t, but not all fields are populated.\n\n"
+   "Entry objects are generated by the 'entries-dump'"
+   " test helper tool.\"\"\"\n\n"
+   "  if b'' == '':\n"
+   "def set_strval(self, name, val):\n"
+   "  self.__setattr__(name, val)\n"
+   "  else:\n"
+   "def set_strval(self, name, val):\n"
+   "  self.__setattr__(name, val.decode('utf-8', "
+   "'surrogateescape'))\n");
+}
+
+static void
+str_value(const char *name, const char *value, apr_pool_t *pool)
 {
   if (value == NULL)
 printf("e.%s = None\n", name);
   else
-printf("e.%s = '%s'\n", name, value);
+{
+  svn_stringbuf_t *escaped_value = NULL;
+  svn_xml_escape_attr_cstring(&escaped_value, value, pool);
+
+  /* Print the human-readable value. */
+  assert(NULL == strchr(escaped_value->data, '\n'));
+  printf("# e.%s = %s\n", name, escaped_value->data);
+
+  /* Print the machine-readable value. */
+  printf("e.set_strval('%s', b'", name);
+  while(*value)
+{
+  printf("\\x%02x", (unsigned int)(unsigned char)*value++);
+}  
+  printf("')\n");
+}
 }
 
 
@@ -130,38 +165,38 @@ entries_dump(const char *dir_path, svn_w
   SVN_ERR_ASSERT(strcmp(key, entry->name) == 0);
 
   printf("e = Entry()\n");
-  str_value("name", entry->name);
+  str_value("name", entry->name, pool);
   int_value("revision", entry->revision);
-  str_value("url", entry->url);
-  str_value("repos", entry->repos);
-  str_value("uuid", entry->uuid);
+  str_value("url", entry->url, pool);
+  str_value("repos", entry->repos, pool);
+  str_value("uuid", entry->uuid, pool);
   int_value("kind", entry->kind);
   int_value("schedule", entry->schedule);
   bool_value("copied", entry->copied);
   bool_value("deleted", entry->deleted);
   bool_value("absent", entry->absent);
   bool_value("incomplete", entry->incomplete);
-  str_value("copyfrom_url", entry->copyfrom_url);
+  str_value("copyfrom_url", entry->copyfrom_url, pool);
   int_value("copyfrom_rev", entry->copyfrom_rev);
-  str_value("conflict_old", entry->conflict_old);
-  str_value("conflict_new", entry->conflict_new);
-  str_value("conflict_wrk", entry->conflict_wrk);
-  str_value("prejfile", entry->prejfile);
+  str_value("conflict_old", entry->conflict_old, pool);
+  str_value("conflict_new", entry->conflict_new, pool);
+  str_value("conflict_wrk", entry->conflict_wrk, pool);
+  str_value("prejfile", entry->prejfile, pool);
   /* skip: text_time */
   /* skip: prop_time */
   /* skip: checksum */
   int_value("cmt_rev", entry->cmt_rev);
   /* skip: cmt_date */
-  str_value("cmt_author", entry->cmt_author);
-  str_value("lock_token", entry->lock_token);
-  str_value("lock_owner", entry->lock_owner);
-  str_value("lock_comment", entry->lock_comment);
+  str_value(