From 56e99d623d05e35692d022e807a66365b222919b Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 15:07:57 +0100 Subject: [PATCH 01/13] Make sure to use a Python 3 pylint On some systems, such as Ubuntu up to 19.04, `pylint` is for Python 2 and `pylint3` is for Python 3, so we should not use `pylint` even if it's available. Use the Python module instead of the trivial shell wrapper. This way we can make sure to use the correct Python version. Fix #3111 Signed-off-by: Gilles Peskine --- tests/scripts/check-python-files.sh | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/tests/scripts/check-python-files.sh b/tests/scripts/check-python-files.sh index 6b864d264a..cd18518cae 100755 --- a/tests/scripts/check-python-files.sh +++ b/tests/scripts/check-python-files.sh @@ -9,15 +9,10 @@ # Run 'pylint' on Python files for programming errors and helps enforcing # PEP8 coding standards. -# Find the installed version of Pylint. Installed as a distro package this can -# be pylint3 and as a PEP egg, pylint. We prefer pylint over pylint3 -if type pylint >/dev/null 2>/dev/null; then - PYLINT=pylint -elif type pylint3 >/dev/null 2>/dev/null; then - PYLINT=pylint3 +if type python3 >/dev/null 2>/dev/null; then + PYTHON=python3 else - echo 'Pylint was not found.' - exit 1 + PYTHON=python fi -$PYLINT -j 2 scripts/*.py tests/scripts/*.py +$PYTHON -m pylint -j 2 scripts/*.py tests/scripts/*.py From 13c95c4d74436937e653bd4b214fcc333ed6cc70 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 15:09:13 +0100 Subject: [PATCH 02/13] Make check_python_files non-optional in all.sh check_python_files was optional in all.sh because we used to have CI machines where pylint wasn't available. But this had the downside that check_python_files kept breaking because it wasn't checked in the CI. Now our CI has pylint and check_python_files should not be optional. Signed-off-by: Gilles Peskine --- tests/scripts/all.sh | 9 --------- 1 file changed, 9 deletions(-) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 9b69aa204a..93d239ec5c 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1750,15 +1750,6 @@ component_test_zeroize () { unset gdb_disable_aslr } -support_check_python_files () { - # Find the installed version of Pylint. Installed as a distro package this can - # be pylint3 and as a PEP egg, pylint. - if type pylint >/dev/null 2>/dev/null || type pylint3 >/dev/null 2>/dev/null; then - true; - else - false; - fi -} component_check_python_files () { msg "Lint: Python scripts" record_status tests/scripts/check-python-files.sh From ce674a90c5cea0f2a89ece4dab805bfaea789886 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 15:37:00 +0100 Subject: [PATCH 03/13] Clearer code to search for config.h Don't use a function argument as a for loop variable. It worked (mostly) but Pylint frowns on it (redefined-argument-from-local) and I think Pylint has a point. If the configuration file is not found, raise an exception mentioning the search path rather than just its last element. Signed-off-by: Gilles Peskine --- scripts/config.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/config.py b/scripts/config.py index b7a9a080e4..d6eb2e48c1 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -283,9 +283,13 @@ class ConfigFile(Config): def __init__(self, filename=None): """Read the Mbed TLS configuration file.""" if filename is None: - for filename in self.default_path: - if os.path.lexists(filename): + for candidate in self.default_path: + if os.path.lexists(candidate): + filename = candidate break + else: + raise Exception('Mbed TLS configuration file not found', + self.default_path) super().__init__() self.filename = filename self.current_section = 'header' From e22a4dacf76fff5456573729084957a46658bb9d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 15:43:49 +0100 Subject: [PATCH 04/13] Explicit return value from main Rather than sometimes returning an integer, sometimes a boolean and sometimes implicitly returning None, always return 0 for success and 1 for failure. No behavior change for the program as a whole, since the None/True/False values were implicitly converted to the desired numerical value. Signed-off-by: Gilles Peskine --- scripts/config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/config.py b/scripts/config.py index d6eb2e48c1..20521a57a9 100755 --- a/scripts/config.py +++ b/scripts/config.py @@ -452,7 +452,7 @@ if __name__ == '__main__': value = config[args.symbol] if value: sys.stdout.write(value + '\n') - return args.symbol not in config + return 0 if args.symbol in config else 1 elif args.command == 'set': if not args.force and args.symbol not in config.settings: sys.stderr.write("A #define for the symbol {} " @@ -465,6 +465,7 @@ if __name__ == '__main__': else: config.adapt(args.adapter) config.write(args.write) + return 0 # Import modules only used by main only if main is defined and called. # pylint: disable=wrong-import-position From 49f467903f18b4cba483cb3753d8f1d9b68e6cce Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 16:07:40 +0100 Subject: [PATCH 05/13] Pylint: allow if-return-else-return Allow the perfectly reasonable idiom if condition1: return value1 else: return value2 Signed-off-by: Gilles Peskine --- .pylintrc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index 037717e354..9f5b1c4cce 100644 --- a/.pylintrc +++ b/.pylintrc @@ -40,7 +40,12 @@ max-attributes=15 max-module-lines=2000 [MESSAGES CONTROL] -disable= +# * no-else-return: Allow the perfectly reasonable idiom +# if condition1: +# return value1 +# else: +# return value2 +disable=no-else-return [REPORTS] # Don't diplay statistics. Just the facts. From 46c54c0a528469b5a98334db485b9711340003af Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 16:39:30 +0100 Subject: [PATCH 06/13] Pylint: disable logging-format-interpolation warning Pylint warns about things like ``log.info('...'.format(...))``. It insists on ``log.info('...', ...)``. This is of minor utility (mainly a performance gain when there are many messages that use formatting and are below the log level). Some versions of Pylint (including 1.8, which is the version on Ubuntu 18.04) only recognize old-style format strings using '%', and complain about something like ``log.info('{}', foo)`` with logging-too-many-args (Pylint supports new-style formatting if declared globally with logging_format_style under [LOGGING] but this requires Pylint >=2.2). Disable this warning to remain compatible with Pylint 1.8 and not have to change abi_check.py to use %-formats instead of {}-formats when logging. Signed-off-by: Gilles Peskine --- .pylintrc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index 9f5b1c4cce..a5df688d97 100644 --- a/.pylintrc +++ b/.pylintrc @@ -40,12 +40,22 @@ max-attributes=15 max-module-lines=2000 [MESSAGES CONTROL] +# * logging-format-interpolation: Pylint warns about things like +# ``log.info('...'.format(...))``. It insists on ``log.info('...', ...)``. +# This is of minor utility (mainly a performance gain when there are +# many messages that use formatting and are below the log level). +# Some versions of Pylint (including 1.8, which is the version on +# Ubuntu 18.04) only recognize old-style format strings using '%', +# and complain about something like ``log.info('{}', foo)`` with +# logging-too-many-args (Pylint supports new-style formatting if +# declared globally with logging_format_style under [LOGGING] but +# this requires Pylint >=2.2). # * no-else-return: Allow the perfectly reasonable idiom # if condition1: # return value1 # else: # return value2 -disable=no-else-return +disable=logging-format-interpolation,no-else-return [REPORTS] # Don't diplay statistics. Just the facts. From aaee444c68e5545ed2522cc330100ecbcef3eca7 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 16:49:21 +0100 Subject: [PATCH 07/13] Document more methods in Python scripts Signed-off-by: Gilles Peskine --- tests/scripts/check-files.py | 19 +++++++++++++++++++ tests/scripts/check-test-cases.py | 2 ++ 2 files changed, 21 insertions(+) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 6e35f52241..16aebb59a3 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -37,20 +37,31 @@ class FileIssueTracker(object): self.files_with_issues = {} def should_check_file(self, filepath): + """Whether the given file name should be checked. + + Files whose name ends with a string listed in ``self.files_exemptions`` + will not be checked. + """ for files_exemption in self.files_exemptions: if filepath.endswith(files_exemption): return False return True def check_file_for_issue(self, filepath): + """Check the specified file for the issue that this class is for. + + Subclasses must implement this method. + """ raise NotImplementedError def record_issue(self, filepath, line_number): + """Record that an issue was found at the specified location.""" if filepath not in self.files_with_issues.keys(): self.files_with_issues[filepath] = [] self.files_with_issues[filepath].append(line_number) def output_file_issues(self, logger): + """Log all the locations where the issue was found.""" if self.files_with_issues.values(): logger.info(self.heading) for filename, lines in sorted(self.files_with_issues.items()): @@ -70,6 +81,10 @@ class LineIssueTracker(FileIssueTracker): """ def issue_with_line(self, line, filepath): + """Check the specified line for the issue that this class is for. + + Subclasses must implement this method. + """ raise NotImplementedError def check_file_line(self, filepath, line, line_number): @@ -77,6 +92,10 @@ class LineIssueTracker(FileIssueTracker): self.record_issue(filepath, line_number) def check_file_for_issue(self, filepath): + """Check the lines of the specified file. + + Subclasses must implement the ``issue_with_line`` method. + """ with open(filepath, "rb") as f: for i, line in enumerate(iter(f.readline, b"")): self.check_file_line(filepath, line, i + 1) diff --git a/tests/scripts/check-test-cases.py b/tests/scripts/check-test-cases.py index 4abaa68824..35a9987497 100755 --- a/tests/scripts/check-test-cases.py +++ b/tests/scripts/check-test-cases.py @@ -77,6 +77,7 @@ def check_description(results, seen, file_name, line_number, description): seen[description] = line_number def check_test_suite(results, data_file_name): + """Check the test cases in the given unit test data file.""" in_paragraph = False descriptions = {} with open(data_file_name, 'rb') as data_file: @@ -94,6 +95,7 @@ def check_test_suite(results, data_file_name): in_paragraph = True def check_ssl_opt_sh(results, file_name): + """Check the test cases in ssl-opt.sh or a file with a similar format.""" descriptions = {} with open(file_name, 'rb') as file_contents: for line_number, line in enumerate(file_contents, 1): From dd4c1c6fe79eeb9384af211183dde923fcade31a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:20:59 +0100 Subject: [PATCH 08/13] mbedtls_test.py: drop compatibility with Python 2 Python 2 is no longer supported upstream. Actively drop compatibility with Python 2. Removing the inheritance of a class on object pacifies recent versions of Pylint (useless-object-inheritance). Signed-off-by: Gilles Peskine --- tests/scripts/mbedtls_test.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/scripts/mbedtls_test.py b/tests/scripts/mbedtls_test.py index 8f24435bfe..9a58a369ae 100755 --- a/tests/scripts/mbedtls_test.py +++ b/tests/scripts/mbedtls_test.py @@ -1,3 +1,5 @@ +#!/usr/bin/env python3 + # Greentea host test script for Mbed TLS on-target test suite testing. # # Copyright (C) 2018, Arm Limited, All Rights Reserved @@ -46,7 +48,7 @@ class TestDataParserError(Exception): pass -class TestDataParser(object): +class TestDataParser: """ Parses test name, dependencies, test function name and test parameters from the data file. From 184c096e956323600a4b20995079a3d7ff9adc78 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:25:17 +0100 Subject: [PATCH 09/13] Pylint: abide by useless-object-inheritance warnings Inheriting from object is a remainder of Python 2 habits and is just clutter in Python 3. Signed-off-by: Gilles Peskine --- scripts/abi_check.py | 2 +- tests/scripts/check-files.py | 4 ++-- tests/scripts/generate_test_code.py | 2 +- tests/scripts/test_generate_test_code.py | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index e19f2c0c66..c2aca501d6 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -29,7 +29,7 @@ from types import SimpleNamespace import xml.etree.ElementTree as ET -class AbiChecker(object): +class AbiChecker: """API and ABI checker.""" def __init__(self, old_version, new_version, configuration): diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 16aebb59a3..65edecc783 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -17,7 +17,7 @@ import codecs import sys -class FileIssueTracker(object): +class FileIssueTracker: """Base class for file-wide issue tracking. To implement a checker that processes a file as a whole, inherit from @@ -189,7 +189,7 @@ class MergeArtifactIssueTracker(LineIssueTracker): return False -class IntegrityChecker(object): +class IntegrityChecker: """Sanity-check files under the current directory.""" def __init__(self, log_file): diff --git a/tests/scripts/generate_test_code.py b/tests/scripts/generate_test_code.py index 1fff09992c..c0f99f74fa 100755 --- a/tests/scripts/generate_test_code.py +++ b/tests/scripts/generate_test_code.py @@ -208,7 +208,7 @@ class GeneratorInputError(Exception): pass -class FileWrapper(io.FileIO, object): +class FileWrapper(io.FileIO): """ This class extends built-in io.FileIO class with attribute line_no, that indicates line number for the line that is read. diff --git a/tests/scripts/test_generate_test_code.py b/tests/scripts/test_generate_test_code.py index 6d7113e18b..e39b29b831 100755 --- a/tests/scripts/test_generate_test_code.py +++ b/tests/scripts/test_generate_test_code.py @@ -294,7 +294,7 @@ class GenDispatch(TestCase): self.assertEqual(code, expected) -class StringIOWrapper(StringIO, object): +class StringIOWrapper(StringIO): """ file like class to mock file object in tests. """ From 8b022359e88515cc16a26f3498f955fcf02e0efe Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:36:56 +0100 Subject: [PATCH 10/13] Pylint: minor code simplifications Simplify the code in minor ways. Each of this changes fixes a warning from Pylint 2.4 that doesn't appear with Pylint 1.7. Signed-off-by: Gilles Peskine --- tests/scripts/generate_test_code.py | 3 +-- tests/scripts/mbedtls_test.py | 2 +- tests/scripts/test_generate_test_code.py | 10 ++++------ tests/scripts/test_psa_constant_names.py | 2 +- 4 files changed, 7 insertions(+), 10 deletions(-) diff --git a/tests/scripts/generate_test_code.py b/tests/scripts/generate_test_code.py index c0f99f74fa..21f816ea9a 100755 --- a/tests/scripts/generate_test_code.py +++ b/tests/scripts/generate_test_code.py @@ -402,8 +402,7 @@ def parse_dependencies(inp_str): :param inp_str: Input string with macros delimited by ':'. :return: list of dependencies """ - dependencies = [dep for dep in map(validate_dependency, - inp_str.split(':'))] + dependencies = list(map(validate_dependency, inp_str.split(':'))) return dependencies diff --git a/tests/scripts/mbedtls_test.py b/tests/scripts/mbedtls_test.py index 9a58a369ae..709bb1a3e0 100755 --- a/tests/scripts/mbedtls_test.py +++ b/tests/scripts/mbedtls_test.py @@ -262,7 +262,7 @@ class MbedTlsTest(BaseHostTest): data_bytes += bytearray(dependencies) data_bytes += bytearray([function_id, len(parameters)]) for typ, param in parameters: - if typ == 'int' or typ == 'exp': + if typ in ('int', 'exp'): i = int(param, 0) data_bytes += b'I' if typ == 'int' else b'E' self.align_32bit(data_bytes) diff --git a/tests/scripts/test_generate_test_code.py b/tests/scripts/test_generate_test_code.py index e39b29b831..c8e8c5ce14 100755 --- a/tests/scripts/test_generate_test_code.py +++ b/tests/scripts/test_generate_test_code.py @@ -1127,9 +1127,8 @@ Diffie-Hellman selftest dhm_selftest: """ stream = StringIOWrapper('test_suite_ut.function', data) - tests = [(name, test_function, dependencies, args) - for name, test_function, dependencies, args in - parse_test_data(stream)] + # List of (name, function_name, dependencies, args) + tests = list(parse_test_data(stream)) test1, test2, test3, test4 = tests self.assertEqual(test1[0], 'Diffie-Hellman full exchange #1') self.assertEqual(test1[1], 'dhm_do_dhm') @@ -1170,9 +1169,8 @@ dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622" """ stream = StringIOWrapper('test_suite_ut.function', data) - tests = [(name, function_name, dependencies, args) - for name, function_name, dependencies, args in - parse_test_data(stream)] + # List of (name, function_name, dependencies, args) + tests = list(parse_test_data(stream)) test1, test2 = tests self.assertEqual(test1[0], 'Diffie-Hellman full exchange #1') self.assertEqual(test1[1], 'dhm_do_dhm') diff --git a/tests/scripts/test_psa_constant_names.py b/tests/scripts/test_psa_constant_names.py index c02555e880..2c9f058ea2 100755 --- a/tests/scripts/test_psa_constant_names.py +++ b/tests/scripts/test_psa_constant_names.py @@ -453,7 +453,7 @@ def main(): tests.run_all(inputs) tests.report(sys.stdout) if tests.errors: - exit(1) + sys.exit(1) if __name__ == '__main__': main() From 7747efce147846467ab33bf6d4d5334fb1532104 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:39:50 +0100 Subject: [PATCH 11/13] Pylint: allow using pass even when not strictly necessary If we take the trouble of using pass, it's because we think the code is clearer that way. For example, Pylint 2.4 rejects pass in def foo(): """Do nothing.""" pass But relying on a docstring as the sole code is weird, hence the use of pass. Signed-off-by: Gilles Peskine --- .pylintrc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index a5df688d97..9ff6eae048 100644 --- a/.pylintrc +++ b/.pylintrc @@ -55,7 +55,9 @@ max-module-lines=2000 # return value1 # else: # return value2 -disable=logging-format-interpolation,no-else-return +# * unnecessary-pass: If we take the trouble of adding a line with "pass", +# it's because we think the code is clearer that way. +disable=logging-format-interpolation,no-else-return,unnecessary-pass [REPORTS] # Don't diplay statistics. Just the facts. From 1759602b296233cc9a1b751ff33bf468c1efaf48 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:47:06 +0100 Subject: [PATCH 12/13] Pylint: silence locally-disabled/enabled messages If we disable or enable a message locally, it's by design. There's no need to clutter the Pylint output with this information. Signed-off-by: Gilles Peskine --- .pylintrc | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.pylintrc b/.pylintrc index 9ff6eae048..ad25a7ca17 100644 --- a/.pylintrc +++ b/.pylintrc @@ -40,6 +40,9 @@ max-attributes=15 max-module-lines=2000 [MESSAGES CONTROL] +# * locally-disabled, locally-enabled: If we disable or enable a message +# locally, it's by design. There's no need to clutter the Pylint output +# with this information. # * logging-format-interpolation: Pylint warns about things like # ``log.info('...'.format(...))``. It insists on ``log.info('...', ...)``. # This is of minor utility (mainly a performance gain when there are @@ -57,7 +60,7 @@ max-module-lines=2000 # return value2 # * unnecessary-pass: If we take the trouble of adding a line with "pass", # it's because we think the code is clearer that way. -disable=logging-format-interpolation,no-else-return,unnecessary-pass +disable=locally-disabled,locally-enabled,logging-format-interpolation,no-else-return,unnecessary-pass [REPORTS] # Don't diplay statistics. Just the facts. From e0c84ac4d2fbba2822bd0960ca154b654b30da97 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 24 Mar 2020 18:47:53 +0100 Subject: [PATCH 13/13] Pylint: explicitly note why we're doing an unchecked subprocess.run Signed-off-by: Gilles Peskine --- tests/scripts/test_config_script.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/scripts/test_config_script.py b/tests/scripts/test_config_script.py index 40ed9fd9b4..c8fdea5eed 100755 --- a/tests/scripts/test_config_script.py +++ b/tests/scripts/test_config_script.py @@ -92,6 +92,7 @@ def list_presets(options): return re.split(r'[ ,]+', options.presets) else: help_text = subprocess.run([options.script, '--help'], + check=False, # config.pl --help returns 255 stdout=subprocess.PIPE, stderr=subprocess.STDOUT).stdout return guess_presets_from_help(help_text.decode('ascii'))