Merge branch 'tor-github/pr/1339'

15 files changed, 337 insertions, 89 deletions

diff --git a/Makefile.am b/Makefile.am
index 845440b56f..485324fc79 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -478,7 +478,7 @@ version:
 
 .PHONY: autostyle-ifdefs
 autostyle-ifdefs:
-	$(PYTHON) scripts/maint/annotate_ifdef_directives $(OWNED_TOR_C_FILES)
+	$(PYTHON) scripts/maint/annotate_ifdef_directives.py $(OWNED_TOR_C_FILES)
 
 .PHONY: autostyle-ifdefs
 autostyle-operators:
diff --git a/changes/ticket31759 b/changes/ticket31759
new file mode 100644
index 0000000000..f7428f711c
--- /dev/null
+++ b/changes/ticket31759
@@ -0,0 +1,5 @@
+  o Minor features (auto-formatting scripts):
+    - When annotating C macros, never generate a line that our check-spaces
+      script would reject. Closes ticket 31759.
+    - When annotating C macros, try to remove cases of double-negation.
+      Closes ticket 31779.
diff --git a/scripts/maint/annotate_ifdef_directives b/scripts/maint/annotate_ifdef_directives
deleted file mode 100755
index ca267a865e..0000000000
--- a/scripts/maint/annotate_ifdef_directives
+++ /dev/null
@@ -1,74 +0,0 @@
-#!/usr/bin/python
-# Copyright (c) 2017-2019, The Tor Project, Inc.
-# See LICENSE for licensing information
-
-import re
-
-LINE_OBVIOUSNESS_LIMIT = 4
-
-class Problem(Exception):
-    pass
-
-def uncomment(s):
-    s = re.sub(r'//.*','',s)
-    s = re.sub(r'/\*.*','',s)
-    return s.strip()
-
-def translate(f_in, f_out):
-    whole_file = []
-    stack = []
-    cur_level = whole_file
-    lineno = 0
-    for line in f_in:
-        lineno += 1
-        m = re.match(r'\s*#\s*(if|ifdef|ifndef|else|endif|elif)\b\s*(.*)',
-                     line)
-        if not m:
-            f_out.write(line)
-            continue
-        command,rest = m.groups()
-        if command in ("if", "ifdef", "ifndef"):
-            # The #if directive pushes us one level lower on the stack.
-            if command == 'ifdef':
-                rest = "defined(%s)"%uncomment(rest)
-            elif command == 'ifndef':
-                rest = "!defined(%s)"%uncomment(rest)
-            elif rest.endswith("\\"):
-                rest = rest[:-1]+"..."
-
-            rest = uncomment(rest)
-
-            new_level = [ (command, rest, lineno) ]
-            stack.append(cur_level)
-            cur_level = new_level
-            f_out.write(line)
-        elif command in ("else", "elif"):
-            if len(cur_level) == 0 or cur_level[-1][0] == 'else':
-                raise Problem("Unexpected #%s on %d"% (command,lineno))
-            if (len(cur_level) == 1 and command == 'else' and
-                lineno > cur_level[0][2] + LINE_OBVIOUSNESS_LIMIT):
-                f_out.write("#else /* !(%s) */\n"%cur_level[0][1])
-            else:
-                f_out.write(line)
-            cur_level.append((command, rest, lineno))
-        else:
-            assert command == 'endif'
-            if len(stack) == 0:
-                raise Problem("Unmatched #%s on %s"% (command,lineno))
-            if lineno <= cur_level[0][2] + LINE_OBVIOUSNESS_LIMIT:
-                f_out.write(line)
-            elif len(cur_level) == 1 or (
-                    len(cur_level) == 2 and cur_level[1][0] == 'else'):
-                f_out.write("#endif /* %s */\n"%cur_level[0][1])
-            else:
-                f_out.write("#endif /* %s || ... */\n"%cur_level[0][1])
-            cur_level = stack.pop()
-    if len(stack) or cur_level != whole_file:
-        raise Problem("Missing #endif")
-
-import sys,os
-for fn in sys.argv[1:]:
-    with open(fn+"_OUT", 'w') as output_file:
-        translate(open(fn, 'r'), output_file)
-    os.rename(fn+"_OUT", fn)
-
diff --git a/scripts/maint/annotate_ifdef_directives.py b/scripts/maint/annotate_ifdef_directives.py
new file mode 100755
index 0000000000..102128bfa0
--- /dev/null
+++ b/scripts/maint/annotate_ifdef_directives.py
@@ -0,0 +1,317 @@
+#!/usr/bin/python
+# Copyright (c) 2017-2019, The Tor Project, Inc.
+# See LICENSE for licensing information
+
+r"""
+This script iterates over a list of C files. For each file, it looks at the
+#if/#else C macros, and annotates them with comments explaining what they
+match.
+
+For example, it replaces this kind of input...
+
+>>> INPUT = '''
+... #ifdef HAVE_OCELOT
+...   C code here
+... #if MIMSY == BOROGROVE
+...   block 1
+...   block 1
+...   block 1
+...   block 1
+... #else
+...   block 2
+...   block 2
+...   block 2
+...   block 2
+... #endif
+... #endif
+... '''
+
+With this kind of output:
+>>> EXPECTED_OUTPUT = '''
+... #ifdef HAVE_OCELOT
+...   C code here
+... #if MIMSY == BOROGROVE
+...   block 1
+...   block 1
+...   block 1
+...   block 1
+... #else /* !(MIMSY == BOROGROVE) */
+...   block 2
+...   block 2
+...   block 2
+...   block 2
+... #endif /* MIMSY == BOROGROVE */
+... #endif /* defined(HAVE_OCELOT) */
+... '''
+
+Here's how to use it:
+>>> import sys
+>>> if sys.version_info.major < 3: from cStringIO import StringIO
+>>> if sys.version_info.major >= 3: from io import StringIO
+
+>>> OUTPUT = StringIO()
+>>> translate(StringIO(INPUT), OUTPUT)
+>>> assert OUTPUT.getvalue() == EXPECTED_OUTPUT
+
+Note that only #else and #endif lines are annotated.  Existing comments
+on those lines are removed.
+"""
+
+import re
+
+# Any block with fewer than this many lines does not need annotations.
+LINE_OBVIOUSNESS_LIMIT = 4
+
+# Maximum line width.  This includes a terminating newline character.
+#
+# (This is the maximum before encoding, so that if the the operating system
+# uses multiple characers to encode newline, that's still okay.)
+LINE_WIDTH=80
+
+class Problem(Exception):
+    pass
+
+def close_parens_needed(expr):
+    """Return the number of left-parentheses needed to make 'expr'
+       balanced.
+
+    >>> close_parens_needed("1+2")
+    0
+    >>> close_parens_needed("(1 + 2)")
+    0
+    >>> close_parens_needed("(1 + 2")
+    1
+    >>> close_parens_needed("(1 + (2 *")
+    2
+    >>> close_parens_needed("(1 + (2 * 3) + (4")
+    2
+    """
+    return expr.count("(") - expr.count(")")
+
+def truncate_expression(expr, new_width):
+    """Given a parenthesized C expression in 'expr', try to return a new
+       expression that is similar to 'expr', but no more than 'new_width'
+       characters long.
+
+       Try to return an expression with balanced parentheses.
+
+    >>> truncate_expression("1+2+3", 8)
+    '1+2+3'
+    >>> truncate_expression("1+2+3+4+5", 8)
+    '1+2+3...'
+    >>> truncate_expression("(1+2+3+4)", 8)
+    '(1+2...)'
+    >>> truncate_expression("(1+(2+3+4))", 8)
+    '(1+...)'
+    >>> truncate_expression("(((((((((", 8)
+    '((...))'
+    """
+    if len(expr) <= new_width:
+        # The expression is already short enough.
+        return expr
+
+    ellipsis = "..."
+
+    # Start this at the minimum that we might truncate.
+    n_to_remove = len(expr) + len(ellipsis) - new_width
+
+    # Try removing characters, one by one, until we get something where
+    # re-balancing the parentheses still fits within the limit.
+    while n_to_remove < len(expr):
+        truncated = expr[:-n_to_remove] + ellipsis
+        truncated += ")" * close_parens_needed(truncated)
+        if len(truncated) <= new_width:
+            return truncated
+        n_to_remove += 1
+
+    return ellipsis
+
+def commented_line(fmt, argument, maxwidth=LINE_WIDTH):
+    # (This is a raw docstring so that our doctests can use \.)
+    r"""
+    Return fmt%argument, for use as a commented line.  If the line would
+    be longer than maxwidth, truncate argument but try to keep its
+    parentheses balanced.
+
+    Requires that fmt%"..." will fit into maxwidth characters.
+
+    Requires that fmt ends with a newline.
+
+    >>> commented_line("/* %s */\n", "hello world", 32)
+    '/* hello world */\n'
+    >>> commented_line("/* %s */\n", "hello world", 15)
+    '/* hello... */\n'
+    >>> commented_line("#endif /* %s */\n", "((1+2) && defined(FOO))", 32)
+    '#endif /* ((1+2) && defi...) */\n'
+
+
+    The default line limit is 80 characters including the newline:
+
+    >>> long_argument = "long " * 100
+    >>> long_line = commented_line("#endif /* %s */\n", long_argument)
+    >>> len(long_line)
+    80
+
+    >>> long_line[:40]
+    '#endif /* long long long long long long '
+    >>> long_line[40:]
+    'long long long long long long lon... */\n'
+
+    If a line works out to being 80 characters naturally, it isn't truncated,
+    and no ellipsis is added.
+
+    >>> medium_argument = "a"*66
+    >>> medium_line = commented_line("#endif /* %s */\n", medium_argument)
+    >>> len(medium_line)
+    80
+    >>> "..." in medium_line
+    False
+    >>> medium_line[:40]
+    '#endif /* aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'
+    >>> medium_line[40:]
+    'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa */\n'
+
+
+    """
+    assert fmt.endswith("\n")
+    result = fmt % argument
+    if len(result) <= maxwidth:
+        return result
+    else:
+        # How long can we let the argument be?  Try filling in the
+        # format with an empty argument to find out.
+        max_arg_width = maxwidth - len(fmt % "")
+        result = fmt % truncate_expression(argument, max_arg_width)
+        assert len(result) <= maxwidth
+        return result
+
+def negate(expr):
+    """Return a negated version of expr; try to avoid double-negation.
+
+    We usually wrap expressions in parentheses and add a "!".
+    >>> negate("A && B")
+    '!(A && B)'
+
+    But if we recognize the expression as negated, we can restore it.
+    >>> negate(negate("A && B"))
+    'A && B'
+
+    The same applies for defined(FOO).
+    >>> negate("defined(FOO)")
+    '!defined(FOO)'
+    >>> negate(negate("defined(FOO)"))
+    'defined(FOO)'
+
+    Internal parentheses don't confuse us:
+    >>> negate("!(FOO) && !(BAR)")
+    '!(!(FOO) && !(BAR))'
+
+    """
+    expr = expr.strip()
+    # See whether we match !(...), with no intervening close-parens.
+    m = re.match(r'^!\s*\(([^\)]*)\)$', expr)
+    if m:
+        return m.group(1)
+
+
+    # See whether we match !?defined(...), with no intervening close-parens.
+    m = re.match(r'^(!?)\s*(defined\([^\)]*\))$', expr)
+    if m:
+        if m.group(1) == "!":
+            prefix = ""
+        else:
+            prefix = "!"
+        return prefix + m.group(2)
+
+    return "!(%s)" % expr
+
+def uncomment(s):
+    """
+    Remove existing trailing comments from an #else or #endif line.
+    """
+    s = re.sub(r'//.*','',s)
+    s = re.sub(r'/\*.*','',s)
+    return s.strip()
+
+def translate(f_in, f_out):
+    """
+    Read a file from f_in, and write its annotated version to f_out.
+    """
+    # A stack listing our current if/else state.  Each member of the stack
+    # is a list of directives.  Each directive is a 3-tuple of
+    #    (command, rest, lineno)
+    # where "command" is one of if/ifdef/ifndef/else/elif, and where
+    # "rest" is an expression in a format suitable for use with #if, and where
+    # lineno is the line number where the directive occurred.
+    stack = []
+    # the stack element corresponding to the top level of the file.
+    whole_file = []
+    cur_level = whole_file
+    lineno = 0
+    for line in f_in:
+        lineno += 1
+        m = re.match(r'\s*#\s*(if|ifdef|ifndef|else|endif|elif)\b\s*(.*)',
+                     line)
+        if not m:
+            # no directive, so we can just write it out.
+            f_out.write(line)
+            continue
+        command,rest = m.groups()
+        if command in ("if", "ifdef", "ifndef"):
+            # The #if directive pushes us one level lower on the stack.
+            if command == 'ifdef':
+                rest = "defined(%s)"%uncomment(rest)
+            elif command == 'ifndef':
+                rest = "!defined(%s)"%uncomment(rest)
+            elif rest.endswith("\\"):
+                rest = rest[:-1]+"..."
+
+            rest = uncomment(rest)
+
+            new_level = [ (command, rest, lineno) ]
+            stack.append(cur_level)
+            cur_level = new_level
+            f_out.write(line)
+        elif command in ("else", "elif"):
+            # We stay at the same level on the stack.  If we have an #else,
+            # we comment it.
+            if len(cur_level) == 0 or cur_level[-1][0] == 'else':
+                raise Problem("Unexpected #%s on %d"% (command,lineno))
+            if (len(cur_level) == 1 and command == 'else' and
+                lineno > cur_level[0][2] + LINE_OBVIOUSNESS_LIMIT):
+                f_out.write(commented_line("#else /* %s */\n",
+                                           negate(cur_level[0][1])))
+            else:
+                f_out.write(line)
+            cur_level.append((command, rest, lineno))
+        else:
+            # We pop one element on the stack, and comment an endif.
+            assert command == 'endif'
+            if len(stack) == 0:
+                raise Problem("Unmatched #%s on %s"% (command,lineno))
+            if lineno <= cur_level[0][2] + LINE_OBVIOUSNESS_LIMIT:
+                f_out.write(line)
+            elif len(cur_level) == 1 or (
+                    len(cur_level) == 2 and cur_level[1][0] == 'else'):
+                f_out.write(commented_line("#endif /* %s */\n",
+                                           cur_level[0][1]))
+            else:
+                f_out.write(commented_line("#endif /* %s || ... */\n",
+                                           cur_level[0][1]))
+            cur_level = stack.pop()
+    if len(stack) or cur_level != whole_file:
+        raise Problem("Missing #endif")
+
+if __name__ == '__main__':
+
+    import sys,os
+
+    if sys.argv[1] == "--self-test":
+        import doctest
+        doctest.testmod()
+        sys.exit(0)
+
+    for fn in sys.argv[1:]:
+        with open(fn+"_OUT", 'w') as output_file:
+            translate(open(fn, 'r'), output_file)
+        os.rename(fn+"_OUT", fn)
diff --git a/src/app/config/config.c b/src/app/config/config.c
index bdfa547fd7..6ee818ab0c 100644
--- a/src/app/config/config.c
+++ b/src/app/config/config.c
@@ -1455,7 +1455,7 @@ options_act_reversible(const or_options_t *old_options, char **msg)
                       "on this OS/with this build.");
     goto rollback;
   }
-#else /* !(!defined(HAVE_SYS_UN_H)) */
+#else /* defined(HAVE_SYS_UN_H) */
   if (options->ControlSocketsGroupWritable && !options->ControlSocket) {
     *msg = tor_strdup("Setting ControlSocketGroupWritable without setting"
                       "a ControlSocket makes no sense.");
@@ -5101,7 +5101,7 @@ find_torrc_filename(config_line_t *cmd_arg,
       } else {
         fname = dflt ? tor_strdup(dflt) : NULL;
       }
-#else /* !(!defined(_WIN32)) */
+#else /* defined(_WIN32) */
       fname = dflt ? tor_strdup(dflt) : NULL;
 #endif /* !defined(_WIN32) */
     }
@@ -8425,7 +8425,7 @@ init_cookie_authentication(const char *fname, const char *header,
       log_warn(LD_FS,"Unable to make %s group-readable.", escaped(fname));
     }
   }
-#else /* !(!defined(_WIN32)) */
+#else /* defined(_WIN32) */
   (void) group_readable;
 #endif /* !defined(_WIN32) */
 
diff --git a/src/lib/crypt_ops/compat_openssl.h b/src/lib/crypt_ops/compat_openssl.h
index 9c10386c34..61ca51315f 100644
--- a/src/lib/crypt_ops/compat_openssl.h
+++ b/src/lib/crypt_ops/compat_openssl.h
@@ -45,7 +45,7 @@
    ((st) == SSL3_ST_SW_SRVR_HELLO_B))
 #define OSSL_HANDSHAKE_STATE int
 #define CONST_IF_OPENSSL_1_1_API
-#else /* !(!defined(OPENSSL_1_1_API)) */
+#else /* defined(OPENSSL_1_1_API) */
 #define STATE_IS_SW_SERVER_HELLO(st) \
   ((st) == TLS_ST_SW_SRVR_HELLO)
 #define CONST_IF_OPENSSL_1_1_API const
diff --git a/src/lib/fs/dir.c b/src/lib/fs/dir.c
index 3c31e00d99..291f1bbf04 100644
--- a/src/lib/fs/dir.c
+++ b/src/lib/fs/dir.c
@@ -262,7 +262,7 @@ check_private_dir,(const char *dirname, cpd_check_t check,
     }
   }
   close(fd);
-#else /* !(!defined(_WIN32)) */
+#else /* defined(_WIN32) */
   /* Win32 case: we can't open() a directory. */
   (void)effective_user;
 
diff --git a/src/lib/log/util_bug.h b/src/lib/log/util_bug.h
index 546ae1e3ef..d7f01618e8 100644
--- a/src/lib/log/util_bug.h
+++ b/src/lib/log/util_bug.h
@@ -96,7 +96,7 @@
   (void)(a);                                                            \
   (void)(fmt);                                                          \
   STMT_END
-#else /* !(defined(TOR_UNIT_TESTS) && ... */
+#else /* !(defined(TOR_UNIT_TESTS) && defined(DISABLE_ASSERTS_IN_UNIT_TES... */
 /** Like assert(3), but send assertion failures to the log as well as to
  * stderr. */
 #define tor_assert(expr) tor_assertf(expr, NULL)
diff --git a/src/lib/math/fp.c b/src/lib/math/fp.c
index 616e4f15c0..49a2a6a2ca 100644
--- a/src/lib/math/fp.c
+++ b/src/lib/math/fp.c
@@ -75,7 +75,7 @@ clamp_double_to_int64(double number)
 */
 #define PROBLEMATIC_FLOAT_CONVERSION_WARNING
 DISABLE_GCC_WARNING(float-conversion)
-#endif /* defined(MINGW_ANY) && GCC_VERSION >= 409 */
+#endif /* (defined(MINGW_ANY)||defined(__FreeBSD__)) && GCC_VERSION >= 409 */
 
 /*
   With clang 4.0 we apparently run into "double promotion" warnings here,
diff --git a/src/lib/memarea/memarea.c b/src/lib/memarea/memarea.c
index 84c73b0b95..0a88210906 100644
--- a/src/lib/memarea/memarea.c
+++ b/src/lib/memarea/memarea.c
@@ -315,7 +315,7 @@ memarea_assert_ok(memarea_t *area)
   }
 }
 
-#else /* !(!defined(DISABLE_MEMORY_SENTINELS)) */
+#else /* defined(DISABLE_MEMORY_SENTINELS) */
 
 struct memarea_t {
   smartlist_t *pieces;
diff --git a/src/lib/process/daemon.c b/src/lib/process/daemon.c
index 3b90bef671..ae34b5bcb8 100644
--- a/src/lib/process/daemon.c
+++ b/src/lib/process/daemon.c
@@ -165,7 +165,7 @@ finish_daemon(const char *desired_cwd)
 
   return 0;
 }
-#else /* !(!defined(_WIN32)) */
+#else /* defined(_WIN32) */
 /* defined(_WIN32) */
 int
 start_daemon(void)
diff --git a/src/lib/process/process.c b/src/lib/process/process.c
index 631c7169f1..2194a603ff 100644
--- a/src/lib/process/process.c
+++ b/src/lib/process/process.c
@@ -513,7 +513,7 @@ process_get_unix_process(const process_t *process)
   tor_assert(process->unix_process);
   return process->unix_process;
 }
-#else /* !(!defined(_WIN32)) */
+#else /* defined(_WIN32) */
 /** Get the internal handle for Windows backend. */
 process_win32_t *
 process_get_win32_process(const process_t *process)
diff --git a/src/lib/process/restrict.c b/src/lib/process/restrict.c
index 534b39d101..93d06de9a2 100644
--- a/src/lib/process/restrict.c
+++ b/src/lib/process/restrict.c
@@ -214,7 +214,7 @@ set_max_file_descriptors(rlim_t limit, int *max_out)
     return -1;
   }
   limit = MAX_CONNECTIONS;
-#else /* !(!defined(HAVE_GETRLIMIT)) */
+#else /* defined(HAVE_GETRLIMIT) */
   struct rlimit rlim;
 
   if (getrlimit(RLIMIT_NOFILE, &rlim) != 0) {
diff --git a/src/lib/process/setuid.c b/src/lib/process/setuid.c
index 6e8258f279..e132787943 100644
--- a/src/lib/process/setuid.c
+++ b/src/lib/process/setuid.c
@@ -376,7 +376,7 @@ switch_id(const char *user, const unsigned flags)
 #endif /* defined(__linux__) && defined(HAVE_SYS_PRCTL_H) && ... */
   return 0;
 
-#else /* !(!defined(_WIN32)) */
+#else /* defined(_WIN32) */
   (void)user;
   (void)flags;
 
diff --git a/src/lib/tls/tortls_openssl.c b/src/lib/tls/tortls_openssl.c
index 86f0ac42cc..58a7b20dec 100644
--- a/src/lib/tls/tortls_openssl.c
+++ b/src/lib/tls/tortls_openssl.c
@@ -657,7 +657,7 @@ tor_tls_context_new(crypto_pk_t *identity, unsigned int key_lifetime,
     if (r < 0)
       goto error;
   }
-#else /* !(defined(SSL_CTX_set1_groups_list) || ...) */
+#else /* !(defined(SSL_CTX_set1_groups_list) || defined(HAVE_SSL_CTX_SET1... */
   if (! is_client) {
     int nid;
     EC_KEY *ec_key;
@@ -673,7 +673,7 @@ tor_tls_context_new(crypto_pk_t *identity, unsigned int key_lifetime,
       SSL_CTX_set_tmp_ecdh(result->ctx, ec_key);
     EC_KEY_free(ec_key);
   }
-#endif /* defined(SSL_CTX_set1_groups_list) || ...) */
+#endif /* defined(SSL_CTX_set1_groups_list) || defined(HAVE_SSL_CTX_SET1_... */
   SSL_CTX_set_verify(result->ctx, SSL_VERIFY_PEER,
                      always_accept_verify_cb);
   /* let us realloc bufs that we're writing from */