From 7844e79b037caa7f29b8ac25745835d402745908 Mon Sep 17 00:00:00 2001 From: Adam Simpkins Date: Wed, 31 Jul 2019 20:53:07 -0700 Subject: [PATCH] fail if unknown variables are used in a manifest Summary: Check that all variable names are valid when loading manifest files. This ensures that getdeps.py will error out if someone makes a typo in a variable name, rather than treating it as never equal to anything. Reviewed By: pkaush Differential Revision: D16477397 fbshipit-source-id: 030e0642ff4a08db8eb74a0a0223e03d53e4880f --- build/fbcode_builder/getdeps/expr.py | 9 ++++++--- build/fbcode_builder/getdeps/manifest.py | 4 +++- .../fbcode_builder/getdeps/test/expr_test.py | 20 ++++++++++++++----- .../getdeps/test/manifest_test.py | 14 ++++++------- 4 files changed, 31 insertions(+), 16 deletions(-) diff --git a/build/fbcode_builder/getdeps/expr.py b/build/fbcode_builder/getdeps/expr.py index 073706f4c..39fd3022b 100644 --- a/build/fbcode_builder/getdeps/expr.py +++ b/build/fbcode_builder/getdeps/expr.py @@ -12,7 +12,7 @@ import re import shlex -def parse_expr(expr_text): +def parse_expr(expr_text, valid_variables): """ parses the simple criteria expression syntax used in dependency specifications. Returns an ExprNode instance that can be evaluated like this: @@ -37,7 +37,7 @@ def parse_expr(expr_text): # none of them evaluated true. """ - p = Parser(expr_text) + p = Parser(expr_text, valid_variables) return p.parse() @@ -112,9 +112,10 @@ class EqualExpr(ExprNode): class Parser(object): - def __init__(self, text): + def __init__(self, text, valid_variables): self.text = text self.lex = shlex.shlex(text) + self.valid_variables = valid_variables def parse(self): expr = self.top() @@ -141,6 +142,8 @@ class Parser(object): return func() if op == "=": + if name not in self.valid_variables: + raise Exception("unknown variable %r in expression" % (name,)) return EqualExpr(name, self.lex.get_token()) raise Exception( diff --git a/build/fbcode_builder/getdeps/manifest.py b/build/fbcode_builder/getdeps/manifest.py index c4c303efc..41e637507 100644 --- a/build/fbcode_builder/getdeps/manifest.py +++ b/build/fbcode_builder/getdeps/manifest.py @@ -88,10 +88,12 @@ ALLOWED_EXPR_SECTIONS = [ "install.files", ] +ALLOWED_VARIABLES = {"os", "distro", "distro_vers", "fb", "test"} + def parse_conditional_section_name(name, section_def): expr = name[len(section_def) + 1 :] - return parse_expr(expr) + return parse_expr(expr, ALLOWED_VARIABLES) def validate_allowed_fields(file_name, section, config, allowed_fields): diff --git a/build/fbcode_builder/getdeps/test/expr_test.py b/build/fbcode_builder/getdeps/test/expr_test.py index 66b7a51b9..37e714bb0 100644 --- a/build/fbcode_builder/getdeps/test/expr_test.py +++ b/build/fbcode_builder/getdeps/test/expr_test.py @@ -15,28 +15,38 @@ from ..expr import parse_expr class ExprTest(unittest.TestCase): def test_equal(self): - e = parse_expr("foo=bar") + valid_variables = {"foo", "some_var", "another_var"} + e = parse_expr("foo=bar", valid_variables) self.assertTrue(e.eval({"foo": "bar"})) self.assertFalse(e.eval({"foo": "not-bar"})) self.assertFalse(e.eval({"not-foo": "bar"})) def test_not_equal(self): - e = parse_expr("not(foo=bar)") + valid_variables = {"foo"} + e = parse_expr("not(foo=bar)", valid_variables) self.assertFalse(e.eval({"foo": "bar"})) self.assertTrue(e.eval({"foo": "not-bar"})) def test_bad_not(self): + valid_variables = {"foo"} with self.assertRaises(Exception): - parse_expr("foo=not(bar)") + parse_expr("foo=not(bar)", valid_variables) + + def test_bad_variable(self): + valid_variables = {"bar"} + with self.assertRaises(Exception): + parse_expr("foo=bar", valid_variables) def test_all(self): - e = parse_expr("all(foo = bar, baz = qux)") + valid_variables = {"foo", "baz"} + e = parse_expr("all(foo = bar, baz = qux)", valid_variables) self.assertTrue(e.eval({"foo": "bar", "baz": "qux"})) self.assertFalse(e.eval({"foo": "bar", "baz": "nope"})) self.assertFalse(e.eval({"foo": "nope", "baz": "nope"})) def test_any(self): - e = parse_expr("any(foo = bar, baz = qux)") + valid_variables = {"foo", "baz"} + e = parse_expr("any(foo = bar, baz = qux)", valid_variables) self.assertTrue(e.eval({"foo": "bar", "baz": "qux"})) self.assertTrue(e.eval({"foo": "bar", "baz": "nope"})) self.assertFalse(e.eval({"foo": "nope", "baz": "nope"})) diff --git a/build/fbcode_builder/getdeps/test/manifest_test.py b/build/fbcode_builder/getdeps/test/manifest_test.py index b43ac62ed..b91903462 100644 --- a/build/fbcode_builder/getdeps/test/manifest_test.py +++ b/build/fbcode_builder/getdeps/test/manifest_test.py @@ -142,16 +142,16 @@ a b c -[dependencies.foo=bar] +[dependencies.test=on] foo """, ) self.assertEqual(p.get_section_as_args("dependencies"), ["a", "b", "c"]) self.assertEqual( - p.get_section_as_args("dependencies", {"foo": "not-bar"}), ["a", "b", "c"] + p.get_section_as_args("dependencies", {"test": "off"}), ["a", "b", "c"] ) self.assertEqual( - p.get_section_as_args("dependencies", {"foo": "bar"}), + p.get_section_as_args("dependencies", {"test": "on"}), ["a", "b", "c", "foo"], ) @@ -180,13 +180,13 @@ name = foo [cmake.defines] foo = bar -[cmake.defines.bar=baz] +[cmake.defines.test=on] foo = baz """, ) self.assertEqual(p.get_section_as_dict("cmake.defines"), {"foo": "bar"}) self.assertEqual( - p.get_section_as_dict("cmake.defines", {"bar": "baz"}), {"foo": "baz"} + p.get_section_as_dict("cmake.defines", {"test": "on"}), {"foo": "baz"} ) p2 = ManifestParser( @@ -195,7 +195,7 @@ foo = baz [manifest] name = foo -[cmake.defines.bar=baz] +[cmake.defines.test=on] foo = baz [cmake.defines] @@ -203,7 +203,7 @@ foo = bar """, ) self.assertEqual( - p2.get_section_as_dict("cmake.defines", {"bar": "baz"}), + p2.get_section_as_dict("cmake.defines", {"test": "on"}), {"foo": "bar"}, msg="sections cascade in the order they appear in the manifest", )