From 475749351dcf7a89ad921f120a6daac80310edde Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 11:30:22 -0400 Subject: [PATCH 01/14] Move the executable part of checkIncludes.py inside an if block. I'll want to make this block into a series of functions in a subsequent commit, but I'm doing this separately to get the indentation change out of the way. This branch will end up with making checkIncludes.py an integrated part of practracker, for ticket 31176. --- scripts/maint/checkIncludes.py | 77 +++++++++++++++++----------------- 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/scripts/maint/checkIncludes.py b/scripts/maint/checkIncludes.py index ec9350b9b1..a6672b0977 100755 --- a/scripts/maint/checkIncludes.py +++ b/scripts/maint/checkIncludes.py @@ -134,48 +134,49 @@ def load_include_rules(fname): result.addPattern(line) return result -list_unused = False -log_sorted_levels = False +if __name__ == '__main__': + list_unused = False + log_sorted_levels = False -uses_dirs = { } + uses_dirs = { } -for dirpath, dirnames, fnames in os.walk("src"): - if ".may_include" in fnames: - rules = load_include_rules(os.path.join(dirpath, RULES_FNAME)) - for fname in fnames: - if fname_is_c(fname): - rules.applyToFile(os.path.join(dirpath,fname)) - if list_unused: - rules.noteUnusedRules() + for dirpath, dirnames, fnames in os.walk("src"): + if ".may_include" in fnames: + rules = load_include_rules(os.path.join(dirpath, RULES_FNAME)) + for fname in fnames: + if fname_is_c(fname): + rules.applyToFile(os.path.join(dirpath,fname)) + if list_unused: + rules.noteUnusedRules() - uses_dirs[rules.incpath] = rules.getAllowedDirectories() + uses_dirs[rules.incpath] = rules.getAllowedDirectories() -if trouble: - err( -"""To change which includes are allowed in a C file, edit the {} -files in its enclosing directory.""".format(RULES_FNAME)) - sys.exit(1) + if trouble: + err( + """To change which includes are allowed in a C file, edit the {} + files in its enclosing directory.""".format(RULES_FNAME)) + sys.exit(1) -all_levels = [] + all_levels = [] -n = 0 -while uses_dirs: - n += 0 - cur_level = [] - for k in list(uses_dirs): - uses_dirs[k] = [ d for d in uses_dirs[k] - if (d in uses_dirs and d != k)] - if uses_dirs[k] == []: - cur_level.append(k) - for k in cur_level: - del uses_dirs[k] - n += 1 - if cur_level and log_sorted_levels: - print(n, cur_level) - if n > 100: - break + n = 0 + while uses_dirs: + n += 0 + cur_level = [] + for k in list(uses_dirs): + uses_dirs[k] = [ d for d in uses_dirs[k] + if (d in uses_dirs and d != k)] + if uses_dirs[k] == []: + cur_level.append(k) + for k in cur_level: + del uses_dirs[k] + n += 1 + if cur_level and log_sorted_levels: + print(n, cur_level) + if n > 100: + break -if uses_dirs: - print("There are circular .may_include dependencies in here somewhere:", - uses_dirs) - sys.exit(1) + if uses_dirs: + print("There are circular .may_include dependencies in here somewhere:", + uses_dirs) + sys.exit(1) From 3f35ac772b0428e77c93968fdfc6f4add1c9f15d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 11:35:13 -0400 Subject: [PATCH 02/14] checkIncludes: introduce rules-file caching. We'll want this so that we can have each file evaluated independently, rather than a directory at a time. --- scripts/maint/checkIncludes.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/scripts/maint/checkIncludes.py b/scripts/maint/checkIncludes.py index a6672b0977..a67397bfaa 100755 --- a/scripts/maint/checkIncludes.py +++ b/scripts/maint/checkIncludes.py @@ -123,8 +123,12 @@ class Rules(object): return allowed +include_rules_cache = {} + def load_include_rules(fname): """ Read a rules file from 'fname', and return it as a Rules object. """ + if fname in include_rules_cache: + return include_rules_cache[fname] result = Rules(os.path.split(fname)[0]) with open_file(fname) as f: for line in f: @@ -132,6 +136,7 @@ def load_include_rules(fname): if line.startswith("#") or not line: continue result.addPattern(line) + include_rules_cache[fname] = result return result if __name__ == '__main__': From 65a69f861e192ae77c7edafe46bc2674bc39b9ea Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 11:49:52 -0400 Subject: [PATCH 03/14] checkIncludes.py: extract topological sort code Our topological sort code really deserves a function of its own. Additionally, don't print from inside the topological sort code: instead, return a result, and let the caller print it. --- scripts/maint/checkIncludes.py | 67 +++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 17 deletions(-) diff --git a/scripts/maint/checkIncludes.py b/scripts/maint/checkIncludes.py index a67397bfaa..c4e77c71e7 100755 --- a/scripts/maint/checkIncludes.py +++ b/scripts/maint/checkIncludes.py @@ -139,6 +139,50 @@ def load_include_rules(fname): include_rules_cache[fname] = result return result +def remove_self_edges(graph): + """Takes a directed graph in as an adjacency mapping (a mapping from + node to a list of the nodes to which it connects). + + Remove all edges from a node to itself.""" + + for k in list(graph): + graph[k] = [ d for d in graph[k] if d != k ] + +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 + perform a topological sort on the graph, arranging the nodes into + "levels", such that every member of each level is only reachable + by members of later levels. + + Returns a list of the members of each level. + + Modifies the input graph, removing every member that could be + sorted. If the graph does not become empty, then it contains a + cycle. + + "limit" is the max depth of the graph after which we give up trying + to sort it and conclude we have a cycle. + """ + all_levels = [] + + n = 0 + while graph: + n += 0 + cur_level = [] + all_levels.append(cur_level) + for k in list(graph): + graph[k] = [ d for d in graph[k] if d in graph ] + if graph[k] == []: + cur_level.append(k) + for k in cur_level: + del graph[k] + n += 1 + if n > limit: + break + + return all_levels + if __name__ == '__main__': list_unused = False log_sorted_levels = False @@ -162,24 +206,13 @@ if __name__ == '__main__': files in its enclosing directory.""".format(RULES_FNAME)) sys.exit(1) - all_levels = [] + remove_self_edges(uses_dirs) + all_levels = toposort(uses_dirs) - n = 0 - while uses_dirs: - n += 0 - cur_level = [] - for k in list(uses_dirs): - uses_dirs[k] = [ d for d in uses_dirs[k] - if (d in uses_dirs and d != k)] - if uses_dirs[k] == []: - cur_level.append(k) - for k in cur_level: - del uses_dirs[k] - n += 1 - if cur_level and log_sorted_levels: - print(n, cur_level) - if n > 100: - break + if log_sorted_levels: + for (n, cur_level) in enumerate(all_levels): + if cur_level: + print(n, cur_level) if uses_dirs: print("There are circular .may_include dependencies in here somewhere:", From 47d9bcfef8cff23225850545400f60a93fe18f49 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 11:58:12 -0400 Subject: [PATCH 04/14] checkIncludes: Separate file-handling from rule-handling This is our shift from directory-at-a-time processing to file-at-a-time processing. --- scripts/maint/checkIncludes.py | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/scripts/maint/checkIncludes.py b/scripts/maint/checkIncludes.py index c4e77c71e7..9daaf13632 100755 --- a/scripts/maint/checkIncludes.py +++ b/scripts/maint/checkIncludes.py @@ -126,9 +126,14 @@ class Rules(object): include_rules_cache = {} def load_include_rules(fname): - """ Read a rules file from 'fname', and return it as a Rules object. """ + """ Read a rules file from 'fname', and return it as a Rules object. + Return 'None' if fname does not exist. + """ if fname in include_rules_cache: return include_rules_cache[fname] + if not os.path.exists(fname): + include_rules_cache[fname] = None + return None result = Rules(os.path.split(fname)[0]) with open_file(fname) as f: for line in f: @@ -139,6 +144,11 @@ def load_include_rules(fname): include_rules_cache[fname] = result return result +def get_all_include_rules(): + return [ rules for (fname,rules) in + sorted(include_rules_cache.items()) + if rules is not None ] + def remove_self_edges(graph): """Takes a directed graph in as an adjacency mapping (a mapping from node to a list of the nodes to which it connects). @@ -187,18 +197,12 @@ if __name__ == '__main__': list_unused = False log_sorted_levels = False - uses_dirs = { } - for dirpath, dirnames, fnames in os.walk("src"): - if ".may_include" in fnames: - rules = load_include_rules(os.path.join(dirpath, RULES_FNAME)) - for fname in fnames: - if fname_is_c(fname): + for fname in fnames: + if fname_is_c(fname): + rules = load_include_rules(os.path.join(dirpath, RULES_FNAME)) + if rules is not None: rules.applyToFile(os.path.join(dirpath,fname)) - if list_unused: - rules.noteUnusedRules() - - uses_dirs[rules.incpath] = rules.getAllowedDirectories() if trouble: err( @@ -206,6 +210,14 @@ if __name__ == '__main__': files in its enclosing directory.""".format(RULES_FNAME)) sys.exit(1) + if list_unused: + for rules in get_all_include_rules(): + rules.noteUnusedRules() + + uses_dirs = { } + for rules in get_all_include_rules(): + uses_dirs[rules.incpath] = rules.getAllowedDirectories() + remove_self_edges(uses_dirs) all_levels = toposort(uses_dirs) From 3f4e89a7abb2a9027d83da48c84227a190f8b31d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 12:25:41 -0400 Subject: [PATCH 05/14] checkIncludes: refactor to use error-iteration style This makes checkIncludes match practracker more closely, and lets us eliminate a global. --- scripts/maint/checkIncludes.py | 50 ++++++++++++++++++++++------------ 1 file changed, 32 insertions(+), 18 deletions(-) diff --git a/scripts/maint/checkIncludes.py b/scripts/maint/checkIncludes.py index 9daaf13632..c398dc7a55 100755 --- a/scripts/maint/checkIncludes.py +++ b/scripts/maint/checkIncludes.py @@ -23,9 +23,6 @@ import os import re import sys -# Global: Have there been any errors? -trouble = False - if sys.version_info[0] <= 2: def open_file(fname): return open(fname, 'r') @@ -36,13 +33,6 @@ else: def warn(msg): print(msg, file=sys.stderr) -def err(msg): - """ Declare that an error has happened, and remember that there has - been an error. """ - global trouble - trouble = True - print(msg, file=sys.stderr) - def fname_is_c(fname): """ Return true iff 'fname' is the name of a file that we should search for possibly disallowed #include directives. """ @@ -65,6 +55,14 @@ def pattern_is_normal(s): return True return False +class Error(object): + def __init__(self, location, msg): + self.location = location + self.msg = msg + + def __str__(self): + return "{} at {}".format(self.msg, self.location) + class Rules(object): """ A 'Rules' object is the parsed version of a .may_include file. """ def __init__(self, dirpath): @@ -88,7 +86,7 @@ class Rules(object): return True return False - def applyToLines(self, lines, context=""): + def applyToLines(self, lines, loc_prefix=""): lineno = 0 for line in lines: lineno += 1 @@ -96,18 +94,19 @@ class Rules(object): if m: include = m.group(1) if not self.includeOk(include): - err("Forbidden include of {} on line {}{}".format( - include, lineno, context)) + yield Error("{}{}".format(loc_prefix,str(lineno)), + "Forbidden include of {}".format(include)) def applyToFile(self, fname): with open_file(fname) as f: #print(fname) - self.applyToLines(iter(f), " of {}".format(fname)) + for error in self.applyToLines(iter(f), "{}:".format(fname)): + yield error def noteUnusedRules(self): for p in self.patterns: if p not in self.usedPatterns: - print("Pattern {} in {} was never used.".format(p, self.dirpath)) + warn("Pattern {} in {} was never used.".format(p, self.dirpath)) def getAllowedDirectories(self): allowed = [] @@ -145,6 +144,8 @@ def load_include_rules(fname): return result def get_all_include_rules(): + """Return a list of all the Rules objects we have loaded so far, + sorted by their directory names.""" return [ rules for (fname,rules) in sorted(include_rules_cache.items()) if rules is not None ] @@ -193,16 +194,29 @@ def toposort(graph, limit=100): return all_levels +def consider_include_rules(fname): + dirpath = os.path.split(fname)[0] + rules_fname = os.path.join(dirpath, RULES_FNAME) + rules = load_include_rules(os.path.join(dirpath, RULES_FNAME)) + if rules is None: + return + + for err in rules.applyToFile(fname): + yield err + if __name__ == '__main__': list_unused = False log_sorted_levels = False + trouble = False + for dirpath, dirnames, fnames in os.walk("src"): for fname in fnames: if fname_is_c(fname): - rules = load_include_rules(os.path.join(dirpath, RULES_FNAME)) - if rules is not None: - rules.applyToFile(os.path.join(dirpath,fname)) + fullpath = os.path.join(dirpath,fname) + for err in consider_include_rules(fullpath): + print(err, file=sys.stderr) + trouble = True if trouble: err( From 9eb12dde181df868932e866c26e39b633aba72c1 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 12:36:13 -0400 Subject: [PATCH 06/14] checkIncludes: add a real main function and CLI --- scripts/maint/checkIncludes.py | 38 +++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/scripts/maint/checkIncludes.py b/scripts/maint/checkIncludes.py index c398dc7a55..c35fcfd856 100755 --- a/scripts/maint/checkIncludes.py +++ b/scripts/maint/checkIncludes.py @@ -204,19 +204,27 @@ def consider_include_rules(fname): for err in rules.applyToFile(fname): yield err -if __name__ == '__main__': + list_unused = False log_sorted_levels = False - trouble = False +def walk_c_files(topdir="src"): + """Run through all c and h files under topdir, looking for + include-rule violations. Yield those violations.""" - for dirpath, dirnames, fnames in os.walk("src"): + for dirpath, dirnames, fnames in os.walk(topdir): for fname in fnames: if fname_is_c(fname): fullpath = os.path.join(dirpath,fname) for err in consider_include_rules(fullpath): - print(err, file=sys.stderr) - trouble = True + yield err + +def run_check_includes(topdir, list_unused=False, log_sorted_levels=False): + trouble = False + + for err in walk_c_files(topdir): + print(err, file=sys.stderr) + trouble = True if trouble: err( @@ -244,3 +252,23 @@ if __name__ == '__main__': print("There are circular .may_include dependencies in here somewhere:", uses_dirs) sys.exit(1) + +def main(argv): + import argparse + + progname = argv[0] + parser = argparse.ArgumentParser(prog=progname) + parser.add_argument("--toposort", action="store_true", + help="Print a topologically sorted list of modules") + parser.add_argument("--list-unused", action="store_true", + help="List unused lines in .may_include files.") + parser.add_argument("topdir", default="src", nargs="?", + help="Top-level directory for the tor source") + args = parser.parse_args(argv[1:]) + + run_check_includes(topdir=args.topdir, + log_sorted_levels=args.toposort, + list_unused=args.list_unused) + +if __name__ == '__main__': + main(sys.argv) From 6fb74753c2d6400ea6d7e118ce2428d0290728a8 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 14:10:40 -0400 Subject: [PATCH 07/14] Move checkIncludes inside practracker Update the makefile accordingly. --- Makefile.am | 4 ++-- scripts/maint/{checkIncludes.py => practracker/includes.py} | 0 2 files changed, 2 insertions(+), 2 deletions(-) rename scripts/maint/{checkIncludes.py => practracker/includes.py} (100%) diff --git a/Makefile.am b/Makefile.am index 8eeba5edb7..e973593bd5 100644 --- a/Makefile.am +++ b/Makefile.am @@ -166,7 +166,7 @@ EXTRA_DIST+= \ Makefile.nmake \ README \ ReleaseNotes \ - scripts/maint/checkIncludes.py \ + scripts/maint/practracker/includes.py \ scripts/maint/checkSpace.pl \ scripts/maint/practracker @@ -366,7 +366,7 @@ endif check-includes: if USEPYTHON - $(PYTHON) $(top_srcdir)/scripts/maint/checkIncludes.py + $(PYTHON) $(top_srcdir)/scripts/maint/practracker/includes.py endif check-best-practices: diff --git a/scripts/maint/checkIncludes.py b/scripts/maint/practracker/includes.py similarity index 100% rename from scripts/maint/checkIncludes.py rename to scripts/maint/practracker/includes.py From 9abbde2c244f40fcf06c34cd72c360a9770fcd7c Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 14:11:51 -0400 Subject: [PATCH 08/14] Update pre-commit hook to find checkIncludes in its new location Also add a temporary script to redirect the hook, if people don't upgrade for a bit. --- scripts/git/pre-commit.git-hook | 4 ++-- scripts/maint/checkIncludes.py | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) create mode 100755 scripts/maint/checkIncludes.py diff --git a/scripts/git/pre-commit.git-hook b/scripts/git/pre-commit.git-hook index 2a29837198..7c7cf88574 100755 --- a/scripts/git/pre-commit.git-hook +++ b/scripts/git/pre-commit.git-hook @@ -36,8 +36,8 @@ elif [ -d src/common ]; then src/tools/*.[ch] fi -if test -e scripts/maint/checkIncludes.py; then - python scripts/maint/checkIncludes.py +if test -e scripts/maint/practracker/includes.py; then + python scripts/maint/practracker/includes.py fi if [ -e scripts/maint/practracker/practracker.py ]; then diff --git a/scripts/maint/checkIncludes.py b/scripts/maint/checkIncludes.py new file mode 100755 index 0000000000..926b201b35 --- /dev/null +++ b/scripts/maint/checkIncludes.py @@ -0,0 +1,14 @@ +#!/usr/bin/python +# Copyright 2018 The Tor Project, Inc. See LICENSE file for licensing info. + +# This file is no longer here; see practracker/includes.py for this +# functionality. This is a stub file that exists so that older git +# hooks will know where to look. + +import sys, os + +dirname = os.path.split(sys.argv[0])[0] +new_location = os.path.join(dirname, "practracker", "includes.py") +python = sys.executable + +os.execl(python, python, new_location, *sys.argv[1:]) From 720951f05664c38155a4515aa76b9c8d3a99269d Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 17:04:00 -0400 Subject: [PATCH 09/14] Teach include-checker about advisory rules A .may_includes file can be "advisory", which means that some violations of the rules are expected. We will track these violations with practracker, not as automatic errors. --- scripts/maint/practracker/includes.py | 24 ++++++++++++----- src/core/crypto/.may_include | 10 +++++++ src/core/mainloop/.may_include | 20 ++++++++++++++ src/core/or/.may_include | 38 +++++++++++++++++++++++++++ src/core/proto/.may_include | 10 +++++++ 5 files changed, 96 insertions(+), 6 deletions(-) create mode 100644 src/core/crypto/.may_include create mode 100644 src/core/mainloop/.may_include create mode 100644 src/core/or/.may_include create mode 100644 src/core/proto/.may_include diff --git a/scripts/maint/practracker/includes.py b/scripts/maint/practracker/includes.py index c35fcfd856..fbd68f4f52 100755 --- a/scripts/maint/practracker/includes.py +++ b/scripts/maint/practracker/includes.py @@ -56,9 +56,10 @@ def pattern_is_normal(s): return False class Error(object): - def __init__(self, location, msg): + def __init__(self, location, msg, is_advisory=False): self.location = location self.msg = msg + self.is_advisory = is_advisory def __str__(self): return "{} at {}".format(self.msg, self.location) @@ -73,8 +74,12 @@ class Rules(object): self.incpath = dirpath self.patterns = [] self.usedPatterns = set() + self.is_advisory = False def addPattern(self, pattern): + if pattern == "!advisory": + self.is_advisory = True + return if not pattern_is_normal(pattern): warn("Unusual pattern {} in {}".format(pattern, self.dirpath)) self.patterns.append(pattern) @@ -95,7 +100,8 @@ class Rules(object): include = m.group(1) if not self.includeOk(include): yield Error("{}{}".format(loc_prefix,str(lineno)), - "Forbidden include of {}".format(include)) + "Forbidden include of {}".format(include), + is_advisory=self.is_advisory) def applyToFile(self, fname): with open_file(fname) as f: @@ -204,7 +210,6 @@ def consider_include_rules(fname): for err in rules.applyToFile(fname): yield err - list_unused = False log_sorted_levels = False @@ -219,12 +224,16 @@ def walk_c_files(topdir="src"): for err in consider_include_rules(fullpath): yield err -def run_check_includes(topdir, list_unused=False, log_sorted_levels=False): +def run_check_includes(topdir, list_unused=False, log_sorted_levels=False, + list_advisories=False): trouble = False for err in walk_c_files(topdir): + if err.is_advisory and not list_advisories: + continue print(err, file=sys.stderr) - trouble = True + if not err.is_advisory: + trouble = True if trouble: err( @@ -262,13 +271,16 @@ def main(argv): help="Print a topologically sorted list of modules") parser.add_argument("--list-unused", action="store_true", 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("topdir", default="src", nargs="?", help="Top-level directory for the tor source") args = parser.parse_args(argv[1:]) run_check_includes(topdir=args.topdir, log_sorted_levels=args.toposort, - list_unused=args.list_unused) + list_unused=args.list_unused, + list_advisories=args.list_advisories) if __name__ == '__main__': main(sys.argv) diff --git a/src/core/crypto/.may_include b/src/core/crypto/.may_include new file mode 100644 index 0000000000..5782a36797 --- /dev/null +++ b/src/core/crypto/.may_include @@ -0,0 +1,10 @@ +!advisory + +orconfig.h + +lib/crypt_ops/*.h +lib/ctime/*.h +lib/cc/*.h +lib/log/*.h + +core/crypto/*.h diff --git a/src/core/mainloop/.may_include b/src/core/mainloop/.may_include new file mode 100644 index 0000000000..79d6a130a4 --- /dev/null +++ b/src/core/mainloop/.may_include @@ -0,0 +1,20 @@ +!advisory + +orconfig.h + +lib/container/*.h +lib/dispatch/*.h +lib/evloop/*.h +lib/pubsub/*.h +lib/subsys/*.h +lib/buf/*.h +lib/crypt_ops/*.h +lib/err/*.h +lib/tls/*.h +lib/net/*.h +lib/evloop/*.h +lib/geoip/*.h +lib/sandbox/*.h +lib/compress/*.h + +core/mainloop/*.h \ No newline at end of file diff --git a/src/core/or/.may_include b/src/core/or/.may_include new file mode 100644 index 0000000000..5173e8a2b6 --- /dev/null +++ b/src/core/or/.may_include @@ -0,0 +1,38 @@ +!advisory + +orconfig.h + +lib/arch/*.h +lib/buf/*.h +lib/cc/*.h +lib/compress/*.h +lib/container/*.h +lib/crypt_ops/*.h +lib/ctime/*.h +lib/defs/*.h +lib/encoding/*.h +lib/err/*.h +lib/evloop/*.h +lib/fs/*.h +lib/geoip/*.h +lib/intmath/*.h +lib/log/*.h +lib/malloc/*.h +lib/math/*.h +lib/net/*.h +lib/pubsub/*.h +lib/string/*.h +lib/subsys/*.h +lib/test/*.h +lib/testsupport/*.h +lib/thread/*.h +lib/time/*.h +lib/tls/*.h +lib/wallclock/*.h + +trunnel/*.h + +core/mainloop/*.h +core/proto/*.h +core/crypto/*.h +core/or/*.h \ No newline at end of file diff --git a/src/core/proto/.may_include b/src/core/proto/.may_include new file mode 100644 index 0000000000..c1647a5cf9 --- /dev/null +++ b/src/core/proto/.may_include @@ -0,0 +1,10 @@ +!advisory + +orconfig.h + +lib/crypt_ops/*.h +lib/buf/*.h + +trunnel/*.h + +core/proto/*.h \ No newline at end of file From 6b26281b50f25f0175f3ba801951324a30ba7a32 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 17:17:50 -0400 Subject: [PATCH 10/14] practracker: a violation of a .may_include rule is now a problem. We treat "0" as the expected number, and warn about everything else. The problem type is "dependency-violation". --- scripts/maint/practracker/practracker.py | 17 ++++++++++++++++- scripts/maint/practracker/problem.py | 17 +++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py index 7e51edb48f..66ebb1277d 100755 --- a/scripts/maint/practracker/practracker.py +++ b/scripts/maint/practracker/practracker.py @@ -25,6 +25,7 @@ import os, sys import metrics import util import problem +import includes # The filename of the exceptions file (it should be placed in the practracker directory) EXCEPTIONS_FNAME = "./exceptions.txt" @@ -35,12 +36,15 @@ MAX_FILE_SIZE = 3000 # lines MAX_FUNCTION_SIZE = 100 # lines # Recommended number of #includes MAX_INCLUDE_COUNT = 50 +# Recommended number of dependency violations +MAX_DEP_VIOLATIONS = 0 # Map from problem type to functions that adjust for tolerance TOLERANCE_FNS = { 'include-count': lambda n: int(n*1.1), 'function-size': lambda n: int(n*1.1), - 'file-size': lambda n: int(n*1.02) + 'file-size': lambda n: int(n*1.02), + 'dependency-violation': lambda n: (n+2) } ####################################################### @@ -94,6 +98,7 @@ def consider_metrics_for_file(fname, f): Yield a sequence of problem.Item objects for all of the metrics in 'f'. """ + real_fname = fname # Strip the useless part of the path if fname.startswith(TOR_TOPDIR): fname = fname[len(TOR_TOPDIR):] @@ -112,6 +117,13 @@ def consider_metrics_for_file(fname, f): for item in consider_function_size(fname, f): yield item + n = 0 + for item in includes.consider_include_rules(real_fname): + n += 1 + if n: + yield problem.DependencyViolationItem(fname, n) + + HEADER="""\ # Welcome to the exceptions file for Tor's best-practices tracker! # @@ -167,6 +179,8 @@ def main(argv): help="Maximum includes per C file") parser.add_argument("--max-function-size", default=MAX_FUNCTION_SIZE, help="Maximum lines per function") + parser.add_argument("--max-dependency-violations", default=MAX_DEP_VIOLATIONS, + help="Maximum number of dependency violations to allow") parser.add_argument("topdir", default=".", nargs="?", help="Top-level directory for the tor source") args = parser.parse_args(argv[1:]) @@ -183,6 +197,7 @@ def main(argv): filt.addThreshold(problem.FileSizeItem("*", int(args.max_file_size))) filt.addThreshold(problem.IncludeCountItem("*", int(args.max_include_count))) filt.addThreshold(problem.FunctionSizeItem("*", int(args.max_function_size))) + filt.addThreshold(problem.DependencyViolationItem("*", int(args.max_dependency_violations))) # 1) Get all the .c files we care about files_list = util.get_tor_c_files(TOR_TOPDIR) diff --git a/scripts/maint/practracker/problem.py b/scripts/maint/practracker/problem.py index 73519d446f..dafcda86f4 100644 --- a/scripts/maint/practracker/problem.py +++ b/scripts/maint/practracker/problem.py @@ -191,6 +191,21 @@ class FunctionSizeItem(Item): def __init__(self, problem_location, metric_value): super(FunctionSizeItem, self).__init__("function-size", problem_location, metric_value) +class DependencyViolationItem(Item): + """ + Denotes a dependency violation in a .c or .h file. A dependency violation + occurs when a file includes a file from some module that is not listed + in its .may_include file. + + The 'problem_location' is the file that contains the problem. + + The 'metric_value' is the number of forbidden includes. + """ + def __init__(self, problem_location, metric_value): + super(DependencyViolationItem, self).__init__("dependency-violation", + problem_location, + metric_value) + comment_re = re.compile(r'#.*$') def get_old_problem_from_exception_str(exception_str): @@ -212,5 +227,7 @@ def get_old_problem_from_exception_str(exception_str): return IncludeCountItem(problem_location, metric_value) elif problem_type == "function-size": return FunctionSizeItem(problem_location, metric_value) + elif problem_type == "dependency-violation": + return DependencyViolationItem(problem_location, metric_value) else: raise ValueError("Unknown exception type {!r}".format(orig_str)) From 2a3c727dfed7465090c2b9bae2f5ac88b3c6dca0 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 17:31:49 -0400 Subject: [PATCH 11/14] Make includes interface more like the rest of practracker Everything else assumes that somebody else will open the file for it. --- scripts/maint/practracker/includes.py | 17 ++++++++--------- scripts/maint/practracker/practracker.py | 4 +++- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/scripts/maint/practracker/includes.py b/scripts/maint/practracker/includes.py index fbd68f4f52..fcc527c983 100755 --- a/scripts/maint/practracker/includes.py +++ b/scripts/maint/practracker/includes.py @@ -103,11 +103,9 @@ class Rules(object): "Forbidden include of {}".format(include), is_advisory=self.is_advisory) - def applyToFile(self, fname): - with open_file(fname) as f: - #print(fname) - for error in self.applyToLines(iter(f), "{}:".format(fname)): - yield error + def applyToFile(self, fname, f): + for error in self.applyToLines(iter(f), "{}:".format(fname)): + yield error def noteUnusedRules(self): for p in self.patterns: @@ -200,14 +198,14 @@ def toposort(graph, limit=100): return all_levels -def consider_include_rules(fname): +def consider_include_rules(fname, f): dirpath = os.path.split(fname)[0] rules_fname = os.path.join(dirpath, RULES_FNAME) rules = load_include_rules(os.path.join(dirpath, RULES_FNAME)) if rules is None: return - for err in rules.applyToFile(fname): + for err in rules.applyToFile(fname, f): yield err list_unused = False @@ -221,8 +219,9 @@ def walk_c_files(topdir="src"): for fname in fnames: if fname_is_c(fname): fullpath = os.path.join(dirpath,fname) - for err in consider_include_rules(fullpath): - yield err + with open(fullpath) as f: + for err in consider_include_rules(fullpath, f): + yield err def run_check_includes(topdir, list_unused=False, log_sorted_levels=False, list_advisories=False): diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py index 66ebb1277d..20c8bb5b4e 100755 --- a/scripts/maint/practracker/practracker.py +++ b/scripts/maint/practracker/practracker.py @@ -117,8 +117,10 @@ def consider_metrics_for_file(fname, f): for item in consider_function_size(fname, f): yield item + # Check for "upward" includes + f.seek(0) n = 0 - for item in includes.consider_include_rules(real_fname): + for item in includes.consider_include_rules(real_fname, f): n += 1 if n: yield problem.DependencyViolationItem(fname, n) From a5971d732eac650d3d5eefd63df1cd4e3a9b13f5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 17:35:20 -0400 Subject: [PATCH 12/14] Move include-violation checking into its own function. --- scripts/maint/practracker/practracker.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/scripts/maint/practracker/practracker.py b/scripts/maint/practracker/practracker.py index 20c8bb5b4e..16ccec3d70 100755 --- a/scripts/maint/practracker/practracker.py +++ b/scripts/maint/practracker/practracker.py @@ -83,6 +83,14 @@ def consider_function_size(fname, f): canonical_function_name = "%s:%s()" % (fname, name) yield problem.FunctionSizeItem(canonical_function_name, lines) +def consider_include_violations(fname, real_fname, f): + n = 0 + for item in includes.consider_include_rules(real_fname, f): + n += 1 + if n: + yield problem.DependencyViolationItem(fname, n) + + ####################################################### def consider_all_metrics(files_list): @@ -119,12 +127,8 @@ def consider_metrics_for_file(fname, f): # Check for "upward" includes f.seek(0) - n = 0 - for item in includes.consider_include_rules(real_fname, f): - n += 1 - if n: - yield problem.DependencyViolationItem(fname, n) - + for item in consider_include_violations(fname, real_fname, f): + yield item HEADER="""\ # Welcome to the exceptions file for Tor's best-practices tracker! From d515b0f4ba5797ab763c43b5ded0a7a5712eae20 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 17:43:53 -0400 Subject: [PATCH 13/14] changes file for ticket 31176 --- changes/ticket31176 | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changes/ticket31176 diff --git a/changes/ticket31176 b/changes/ticket31176 new file mode 100644 index 0000000000..5fcdeab3af --- /dev/null +++ b/changes/ticket31176 @@ -0,0 +1,5 @@ + o Major features (developer tools): + - Our best-practices tracker now integrates with our include-checker tool + to keep track of the layering violations that we have not yet fixed. + We hope to reduce this number over time to improve Tor's modularity. + Closes ticket 31176. From 0f4b245b20e4fc16d4382a48121801c300b686a9 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 5 Aug 2019 17:44:15 -0400 Subject: [PATCH 14/14] update exceptions file for depencency violations --- scripts/maint/practracker/exceptions.txt | 42 ++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/scripts/maint/practracker/exceptions.txt b/scripts/maint/practracker/exceptions.txt index d437726fe8..39eb7a437e 100644 --- a/scripts/maint/practracker/exceptions.txt +++ b/scripts/maint/practracker/exceptions.txt @@ -51,6 +51,11 @@ problem function-size /src/app/main/main.c:tor_init() 133 problem function-size /src/app/main/main.c:sandbox_init_filter() 291 problem function-size /src/app/main/main.c:run_tor_main_loop() 105 problem function-size /src/app/main/ntmain.c:nt_service_install() 126 +problem dependency-violation /src/core/crypto/hs_ntor.c 1 +problem dependency-violation /src/core/crypto/onion_crypto.c 5 +problem dependency-violation /src/core/crypto/onion_fast.c 1 +problem dependency-violation /src/core/crypto/onion_tap.c 3 +problem dependency-violation /src/core/crypto/relay_crypto.c 9 problem file-size /src/core/mainloop/connection.c 5569 problem include-count /src/core/mainloop/connection.c 62 problem function-size /src/core/mainloop/connection.c:connection_free_minimal() 185 @@ -63,32 +68,49 @@ problem function-size /src/core/mainloop/connection.c:connection_handle_read_imp problem function-size /src/core/mainloop/connection.c:connection_buf_read_from_socket() 180 problem function-size /src/core/mainloop/connection.c:connection_handle_write_impl() 241 problem function-size /src/core/mainloop/connection.c:assert_connection_ok() 143 +problem dependency-violation /src/core/mainloop/connection.c 44 +problem dependency-violation /src/core/mainloop/cpuworker.c 12 problem include-count /src/core/mainloop/mainloop.c 63 problem function-size /src/core/mainloop/mainloop.c:conn_close_if_marked() 108 problem function-size /src/core/mainloop/mainloop.c:run_connection_housekeeping() 123 +problem dependency-violation /src/core/mainloop/mainloop.c 49 +problem dependency-violation /src/core/mainloop/mainloop_pubsub.c 1 +problem dependency-violation /src/core/mainloop/mainloop_sys.c 1 +problem dependency-violation /src/core/mainloop/netstatus.c 4 +problem dependency-violation /src/core/mainloop/periodic.c 2 +problem dependency-violation /src/core/or/address_set.c 1 problem file-size /src/core/or/channel.c 3487 +problem dependency-violation /src/core/or/channel.c 9 +problem dependency-violation /src/core/or/channelpadding.c 6 problem function-size /src/core/or/channeltls.c:channel_tls_handle_var_cell() 160 problem function-size /src/core/or/channeltls.c:channel_tls_process_versions_cell() 170 problem function-size /src/core/or/channeltls.c:channel_tls_process_netinfo_cell() 214 problem function-size /src/core/or/channeltls.c:channel_tls_process_certs_cell() 246 problem function-size /src/core/or/channeltls.c:channel_tls_process_authenticate_cell() 202 +problem dependency-violation /src/core/or/channeltls.c 10 problem include-count /src/core/or/circuitbuild.c 54 problem function-size /src/core/or/circuitbuild.c:get_unique_circ_id_by_chan() 128 problem function-size /src/core/or/circuitbuild.c:circuit_extend() 147 problem function-size /src/core/or/circuitbuild.c:choose_good_exit_server_general() 206 +problem dependency-violation /src/core/or/circuitbuild.c 25 problem include-count /src/core/or/circuitlist.c 55 problem function-size /src/core/or/circuitlist.c:HT_PROTOTYPE() 109 problem function-size /src/core/or/circuitlist.c:circuit_free_() 143 problem function-size /src/core/or/circuitlist.c:circuit_find_to_cannibalize() 101 problem function-size /src/core/or/circuitlist.c:circuit_about_to_free() 120 problem function-size /src/core/or/circuitlist.c:circuits_handle_oom() 117 +problem dependency-violation /src/core/or/circuitlist.c 19 problem function-size /src/core/or/circuitmux.c:circuitmux_set_policy() 109 problem function-size /src/core/or/circuitmux.c:circuitmux_attach_circuit() 113 +problem dependency-violation /src/core/or/circuitmux_ewma.c 2 problem file-size /src/core/or/circuitpadding.c 3043 problem function-size /src/core/or/circuitpadding.c:circpad_machine_schedule_padding() 107 +problem dependency-violation /src/core/or/circuitpadding.c 6 problem function-size /src/core/or/circuitpadding_machines.c:circpad_machine_relay_hide_intro_circuits() 103 problem function-size /src/core/or/circuitpadding_machines.c:circpad_machine_client_hide_rend_circuits() 112 +problem dependency-violation /src/core/or/circuitpadding_machines.c 1 problem function-size /src/core/or/circuitstats.c:circuit_build_times_parse_state() 123 +problem dependency-violation /src/core/or/circuitstats.c 11 problem file-size /src/core/or/circuituse.c 3162 problem function-size /src/core/or/circuituse.c:circuit_is_acceptable() 128 problem function-size /src/core/or/circuituse.c:circuit_expire_building() 394 @@ -97,8 +119,10 @@ problem function-size /src/core/or/circuituse.c:circuit_build_failed() 149 problem function-size /src/core/or/circuituse.c:circuit_launch_by_extend_info() 108 problem function-size /src/core/or/circuituse.c:circuit_get_open_circ_or_launch() 352 problem function-size /src/core/or/circuituse.c:connection_ap_handshake_attach_circuit() 244 +problem dependency-violation /src/core/or/circuituse.c 23 problem function-size /src/core/or/command.c:command_process_create_cell() 156 problem function-size /src/core/or/command.c:command_process_relay_cell() 132 +problem dependency-violation /src/core/or/command.c 8 problem file-size /src/core/or/connection_edge.c 4596 problem include-count /src/core/or/connection_edge.c 65 problem function-size /src/core/or/connection_edge.c:connection_ap_expire_beginning() 117 @@ -109,14 +133,21 @@ problem function-size /src/core/or/connection_edge.c:connection_ap_handshake_sen problem function-size /src/core/or/connection_edge.c:connection_ap_handshake_socks_resolved() 101 problem function-size /src/core/or/connection_edge.c:connection_exit_begin_conn() 185 problem function-size /src/core/or/connection_edge.c:connection_exit_connect() 102 +problem dependency-violation /src/core/or/connection_edge.c 27 problem file-size /src/core/or/connection_or.c 3122 problem include-count /src/core/or/connection_or.c 51 problem function-size /src/core/or/connection_or.c:connection_or_group_set_badness_() 105 problem function-size /src/core/or/connection_or.c:connection_or_client_learned_peer_id() 142 problem function-size /src/core/or/connection_or.c:connection_or_compute_authenticate_cell_body() 231 +problem dependency-violation /src/core/or/connection_or.c 20 +problem dependency-violation /src/core/or/dos.c 5 +problem dependency-violation /src/core/or/onion.c 2 +problem dependency-violation /src/core/or/or_periodic.c 1 problem file-size /src/core/or/policies.c 3249 problem function-size /src/core/or/policies.c:policy_summarize() 107 +problem dependency-violation /src/core/or/policies.c 14 problem function-size /src/core/or/protover.c:protover_all_supported() 117 +problem dependency-violation /src/core/or/reasons.c 2 problem file-size /src/core/or/relay.c 3263 problem function-size /src/core/or/relay.c:circuit_receive_relay_cell() 126 problem function-size /src/core/or/relay.c:relay_send_command_from_edge_() 109 @@ -125,10 +156,21 @@ problem function-size /src/core/or/relay.c:connection_edge_process_relay_cell_no problem function-size /src/core/or/relay.c:handle_relay_cell_command() 369 problem function-size /src/core/or/relay.c:connection_edge_package_raw_inbuf() 128 problem function-size /src/core/or/relay.c:circuit_resume_edge_reading_helper() 146 +problem dependency-violation /src/core/or/relay.c 16 +problem dependency-violation /src/core/or/scheduler.c 1 problem function-size /src/core/or/scheduler_kist.c:kist_scheduler_run() 171 +problem dependency-violation /src/core/or/scheduler_kist.c 2 problem function-size /src/core/or/scheduler_vanilla.c:vanilla_scheduler_run() 109 +problem dependency-violation /src/core/or/scheduler_vanilla.c 1 +problem dependency-violation /src/core/or/sendme.c 2 +problem dependency-violation /src/core/or/status.c 12 problem function-size /src/core/or/versions.c:tor_version_parse() 104 +problem dependency-violation /src/core/proto/proto_cell.c 3 +problem dependency-violation /src/core/proto/proto_control0.c 1 +problem dependency-violation /src/core/proto/proto_ext_or.c 2 +problem dependency-violation /src/core/proto/proto_http.c 1 problem function-size /src/core/proto/proto_socks.c:parse_socks_client() 110 +problem dependency-violation /src/core/proto/proto_socks.c 8 problem function-size /src/feature/client/addressmap.c:addressmap_rewrite() 109 problem function-size /src/feature/client/bridges.c:rewrite_node_address_for_bridge() 126 problem function-size /src/feature/client/circpathbias.c:pathbias_measure_close_rate() 108