Ticket31634 v2

changes/ticket31634

@@ -0,0 +1,4 @@
+  o Minor features (testing, architeture):
+    - Our test scripts now double-check that subsystem initialization order
+      is consistent with the inter-module dependencies established by our
+      .may_include files.  Implements ticket 31634.

doc/tor.1.txt

@@ -186,6 +186,13 @@ The following options in this section are only recognized on the
     ISO-8601 format.  For example, the output sent to stdout will be
     of the form: "signing-cert-expiry: 2017-07-25 08:30:15 UTC"
 
+[[opt-dbg]] **--dbg-**...::
+    Tor may support other options beginning with the string "dbg". These
+    are intended for use by developers to debug and test Tor. They are
+    not supported or guaranteed to be stable, and you should probably
+    not use them.
+
+
 [[conf-format]]
 == THE CONFIGURATION FILE FORMAT
 

scripts/maint/practracker/includes.py

@@ -59,6 +59,8 @@ def fname_is_c(fname):
     re.compile(r'^micro-revision.i$'),
 ]
 
+TOPDIR = "src"
+
 def pattern_is_normal(s):
     for p in ALLOWED_PATTERNS:
         if p.match(s):
@@ -136,6 +138,29 @@ def getAllowedDirectories(self):
 
         return allowed
 
+
+def normalize_srcdir(fname):
+    """given the name of a source directory or file, return its name
+        relative to `src` in a unix-like format.
+    """
+    orig = fname
+    dirname, dirfile = os.path.split(fname)
+    if re.match(r'.*\.[ch]$', dirfile):
+        fname = dirname
+
+    # Now we have a directory.
+    dirname, result = os.path.split(fname)
+    for _ in range(100):
+        # prevent excess looping in case I missed a tricky case
+        dirname, dirpart = os.path.split(dirname)
+        if dirpart == 'src' or dirname == "":
+            #print(orig,"=>",result)
+            return result
+        result = "{}/{}".format(dirpart,result)
+
+    print("No progress!")
+    assert False
+
 include_rules_cache = {}
 
 def load_include_rules(fname):
@@ -173,6 +198,27 @@ def remove_self_edges(graph):
     for k in list(graph):
         graph[k] = [ d for d in graph[k] if d != k ]
 
+def closure(graph):
+    """Takes a directed graph in as an adjacency mapping (a mapping from
+       node to a list of the nodes to which it connects), and completes
+       its closure.
+    """
+    graph = graph.copy()
+    changed = False
+    for k in graph.keys():
+        graph[k] = set(graph[k])
+    while True:
+        for k in graph.keys():
+            sz = len(graph[k])
+            for v in list(graph[k]):
+                graph[k].update(graph.get(v, []))
+            if sz != len(graph[k]):
+                changed = True
+
+        if not changed:
+            return graph
+        changed = False
+
 def toposort(graph, limit=100):
     """Takes a directed graph in as an adjacency mapping (a mapping from
        node to a list of the nodes to which it connects).  Tries to
@@ -233,8 +279,38 @@ def walk_c_files(topdir="src"):
                     for err in consider_include_rules(fullpath, f):
                         yield err
 
+def open_or_stdin(fname):
+    if fname == '-':
+        return sys.stdin
+    else:
+        return open(fname)
+
+def check_subsys_file(fname, uses_dirs):
+    if not uses_dirs:
+        # We're doing a distcheck build, or for some other reason there are
+        # no .may_include files.
+        print("SKIPPING")
+        return False
+
+    uses_dirs = { normalize_srcdir(k) : { normalize_srcdir(d) for d in v }
+                  for (k,v) in uses_dirs.items() }
+    uses_closure = closure(uses_dirs)
+    ok = True
+    previous_subsystems = []
+
+    with open_or_stdin(fname) as f:
+        for line in f:
+            _, name, fname = line.split()
+            fname = normalize_srcdir(fname)
+            for prev in previous_subsystems:
+                if fname in uses_closure[prev]:
+                    print("INVERSION: {} uses {}".format(prev,fname))
+                    ok = False
+            previous_subsystems.append(fname)
+    return not ok
+
 def run_check_includes(topdir, list_unused=False, log_sorted_levels=False,
-                       list_advisories=False):
+                       list_advisories=False, check_subsystem_order=None):
     trouble = False
 
     for err in walk_c_files(topdir):
@@ -259,6 +335,11 @@ def run_check_includes(topdir, list_unused=False, log_sorted_levels=False,
         uses_dirs[rules.incpath] = rules.getAllowedDirectories()
 
     remove_self_edges(uses_dirs)
+
+    if check_subsystem_order:
+        if check_subsys_file(check_subsystem_order, uses_dirs):
+            sys.exit(1)
+
     all_levels = toposort(uses_dirs)
 
     if log_sorted_levels:
@@ -282,14 +363,19 @@ def main(argv):
                         help="List unused lines in .may_include files.")
     parser.add_argument("--list-advisories", action="store_true",
                         help="List advisories as well as forbidden includes")
+    parser.add_argument("--check-subsystem-order", action="store",
+                        help="Check a list of subsystems for ordering")
     parser.add_argument("topdir", default="src", nargs="?",
                         help="Top-level directory for the tor source")
     args = parser.parse_args(argv[1:])
 
+    global TOPDIR
+    TOPDIR = args.topdir
     run_check_includes(topdir=args.topdir,
                        log_sorted_levels=args.toposort,
                        list_unused=args.list_unused,
-                       list_advisories=args.list_advisories)
+                       list_advisories=args.list_advisories,
+                       check_subsystem_order=args.check_subsystem_order)
 
 if __name__ == '__main__':
     main(sys.argv)

scripts/maint/run_check_subsystem_order.sh

@@ -0,0 +1,17 @@
+#!/usr/bin/env bash
+
+set -e
+
+TOR="${abs_top_builddir:-.}/src/app/tor"
+
+INCLUDES_PY="${abs_top_srcdir:-.}/scripts/maint/practracker/includes.py"
+
+if ! test -x "${INCLUDES_PY}" ; then
+    echo "skip"
+    exit 77
+fi
+
+"${TOR}" --dbg-dump-subsystem-list | \
+    "${INCLUDES_PY}" --check-subsystem-order - "${abs_top_srcdir}/src"
+
+echo ok

src/app/config/config.c

@@ -2498,6 +2498,9 @@ static const struct {
     .command=CMD_IMMEDIATE },
   { .name="--nt-service" },
   { .name="-nt-service" },
+  { .name="--dbg-dump-subsystem-list",
+    .command=CMD_IMMEDIATE,
+    .quiet=QUIET_HUSH },
   { .name=NULL },
 };
 
@@ -4604,6 +4607,10 @@ options_init_from_torrc(int argc, char **argv)
     list_deprecated_options();
     return 1;
   }
+  if (config_line_find(cmdline_only_options, "--dbg-dump-subsystem-list")) {
+    subsystems_dump_list();
+    return 1;
+  }
 
   if (config_line_find(cmdline_only_options, "--version")) {
     printf("Tor version %s.\n",get_version());

src/app/main/subsysmgr.c

@@ -293,6 +293,20 @@ subsystems_thread_cleanup(void)
   }
 }
 
+/**
+ * Dump a human- and machine-readable list of all the subsystems to stdout,
+ * in their initialization order, prefixed with their level.
+ **/
+void
+subsystems_dump_list(void)
+{
+  for (unsigned i = 0; i < n_tor_subsystems - 1; ++i) {
+    const subsys_fns_t *sys = tor_subsystems[i];
+    printf("% 4d\t%16s\t%s\n", sys->level, sys->name,
+           sys->location?sys->location:"");
+  }
+}
+
 /**
  * Register all subsystem-declared options formats in <b>mgr</b>.
  *

src/app/main/subsysmgr.h

@@ -31,6 +31,8 @@ void subsystems_prefork(void);
 void subsystems_postfork(void);
 void subsystems_thread_cleanup(void);
 
+void subsystems_dump_list(void);
+
 struct config_mgr_t;
 int subsystems_register_options_formats(struct config_mgr_t *mgr);
 int subsystems_register_state_formats(struct config_mgr_t *mgr);

src/core/mainloop/mainloop_sys.c

@@ -78,6 +78,7 @@ mainloop_flush_state(void *arg)
 
 const struct subsys_fns_t sys_mainloop = {
   .name = "mainloop",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = 5,
   .initialize = subsys_mainloop_initialize,

src/core/or/or_sys.c

@@ -47,6 +47,7 @@ subsys_or_add_pubsub(struct pubsub_connector_t *connector)
 
 const struct subsys_fns_t sys_or = {
   .name = "or",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = 20,
   .initialize = subsys_or_initialize,

src/feature/control/btrack.c

@@ -56,6 +56,7 @@ btrack_add_pubsub(pubsub_connector_t *connector)
 
 const subsys_fns_t sys_btrack = {
   .name = "btrack",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = 55,
   .initialize = btrack_init,

src/feature/dirauth/dirauth_stub.c

@@ -26,6 +26,7 @@ static const config_format_t dirauth_options_stub_fmt = {
 
 const struct subsys_fns_t sys_dirauth = {
   .name = "dirauth",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = false,
   .level = DIRAUTH_SUBSYS_LEVEL,
 

src/feature/dirauth/dirauth_sys.c

@@ -60,6 +60,7 @@ dirauth_set_options(void *arg)
 
 const struct subsys_fns_t sys_dirauth = {
   .name = "dirauth",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = DIRAUTH_SUBSYS_LEVEL,
   .initialize = subsys_dirauth_initialize,

src/feature/relay/relay_stub.c

@@ -15,6 +15,7 @@
 
 const struct subsys_fns_t sys_relay = {
   .name = "relay",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = false,
   .level = RELAY_SUBSYS_LEVEL,
 };

src/feature/relay/relay_sys.c

@@ -41,6 +41,7 @@ subsys_relay_shutdown(void)
 
 const struct subsys_fns_t sys_relay = {
   .name = "relay",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = RELAY_SUBSYS_LEVEL,
   .initialize = subsys_relay_initialize,

src/lib/compress/compress.c

@@ -694,6 +694,7 @@ subsys_compress_initialize(void)
 
 const subsys_fns_t sys_compress = {
   .name = "compress",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = -55,
   .initialize = subsys_compress_initialize,

src/lib/crypt_ops/crypto_init.c

@@ -317,6 +317,7 @@ crypto_set_options(void *arg)
 
 const struct subsys_fns_t sys_crypto = {
   .name = "crypto",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = -60,
   .initialize = subsys_crypto_initialize,

src/lib/err/torerr_sys.c

@@ -34,6 +34,7 @@ subsys_torerr_shutdown(void)
 
 const subsys_fns_t sys_torerr = {
   .name = "err",
+  SUBSYS_DECLARE_LOCATION(),
   /* Low-level error handling is a diagnostic feature, we want it to init
    * right after windows process security, and shutdown last.
    * (Security never shuts down.) */

src/lib/evloop/evloop_sys.c

@@ -41,6 +41,7 @@ subsys_evloop_shutdown(void)
 
 const struct subsys_fns_t sys_evloop = {
   .name = "evloop",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = -20,
   .initialize = subsys_evloop_initialize,

src/lib/llharden/winprocess_sys.c

@@ -58,6 +58,7 @@ subsys_winprocess_initialize(void)
 
 const subsys_fns_t sys_winprocess = {
   .name = "winprocess",
+  SUBSYS_DECLARE_LOCATION(),
   /* HeapEnableTerminationOnCorruption and setdeppolicy() are security
    * features, we want them to run first. */
   .level = -100,

src/lib/log/log_sys.c

@@ -28,6 +28,7 @@ subsys_logging_shutdown(void)
 
 const subsys_fns_t sys_logging = {
   .name = "log",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   /* Logging depends on threads, approx time, raw logging, and security.
    * Most other lib modules depend on logging. */

src/lib/net/network_sys.c

@@ -37,6 +37,7 @@ subsys_network_shutdown(void)
 
 const subsys_fns_t sys_network = {
   .name = "network",
+  SUBSYS_DECLARE_LOCATION(),
   /* Network depends on logging, and a lot of other modules depend on network.
    */
   .level = -55,

src/lib/process/process_sys.c

@@ -26,6 +26,7 @@ subsys_process_shutdown(void)
 
 const subsys_fns_t sys_process = {
   .name = "process",
+  SUBSYS_DECLARE_LOCATION(),
   .level = -18,
   .supported = true,
   .initialize = subsys_process_initialize,

src/lib/subsys/subsys.h

@@ -41,6 +41,11 @@ typedef struct subsys_fns_t {
    **/
   const char *name;
 
+  /**
+   * The file in which the subsystem object is declared. Used for debugging.
+   **/
+  const char *location;
+
   /**
    * Whether this subsystem is supported -- that is, whether it is compiled
    * into Tor.  For most subsystems, this should be true.
@@ -187,6 +192,14 @@ typedef struct subsys_fns_t {
   int (*flush_state)(void *);
 } subsys_fns_t;
 
+#ifndef COCCI
+/**
+ * Macro to declare a subsystem's location.
+ **/
+#define SUBSYS_DECLARE_LOCATION() \
+  .location = __FILE__
+#endif
+
 /**
  * Lowest allowed subsystem level.
  **/

src/lib/thread/compat_threads.c

@@ -129,6 +129,7 @@ subsys_threads_initialize(void)
 
 const subsys_fns_t sys_threads = {
   .name = "threads",
+  SUBSYS_DECLARE_LOCATION(),
   .supported = true,
   .level = -89,
   .initialize = subsys_threads_initialize,

src/lib/time/time_sys.c

@@ -20,6 +20,7 @@ subsys_time_initialize(void)
 
 const subsys_fns_t sys_time = {
   .name = "time",
+  SUBSYS_DECLARE_LOCATION(),
   /* Monotonic time depends on logging, and a lot of other modules depend on
    * monotonic time. */
   .level = -80,

src/lib/tls/tortls.c

@@ -456,6 +456,7 @@ subsys_tortls_shutdown(void)
 
 const subsys_fns_t sys_tortls = {
   .name = "tortls",
+  SUBSYS_DECLARE_LOCATION(),
   .level = -50,
   .shutdown = subsys_tortls_shutdown
 };