From 24eec79255f42c8a76c58da6c0862bad66158a56 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Thu, 4 Apr 2019 15:02:01 +0300 Subject: [PATCH 01/32] Add guards for MBEDTLS_X509_CRL_PARSE_C in sample Add checks in `ssl_server2` that `MBEDTLS_X509_CRL_PARSE_C` is defined to fix compilation issue. Fixes #560. --- ChangeLog | 2 ++ programs/ssl/ssl_server2.c | 21 +++++++++++++++++---- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index 3b078c9ce..c074dfad2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -67,6 +67,8 @@ Bugfix extensions in CSRs and CRTs that caused these bitstrings to not be encoded correctly as trailing zeroes were not accounted for as unused bits in the leading content octet. Fixes #1610. + * Add a check for MBEDTLS_X509_CRL_PARSE_C in ssl_server2, guarding the crl + sni entry parameter. Reported by inestlerode in #560. Changes * Include configuration file in all header files that use configuration, diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 2a499adf9..a676f44e5 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -251,8 +251,14 @@ int main( void ) #endif /* MBEDTLS_SSL_CACHE_C */ #if defined(SNI_OPTION) +#if defined(MBEDTLS_X509_CRL_PARSE_C) +#define SNI_CRL ",crl" +#else +#define SNI_CRL "" +#endif + #define USAGE_SNI \ - " sni=%%s name1,cert1,key1,ca1,crl1,auth1[,...]\n" \ + " sni=%%s name1,cert1,key1,ca1"SNI_CRL",auth1[,...]\n" \ " default: disabled\n" #else #define USAGE_SNI "" @@ -619,10 +625,10 @@ void sni_free( sni_entry *head ) mbedtls_x509_crt_free( cur->ca ); mbedtls_free( cur->ca ); - +#if defined(MBEDTLS_X509_CRL_PARSE_C) mbedtls_x509_crl_free( cur->crl ); mbedtls_free( cur->crl ); - +#endif next = cur->next; mbedtls_free( cur ); cur = next; @@ -641,7 +647,10 @@ sni_entry *sni_parse( char *sni_string ) sni_entry *cur = NULL, *new = NULL; char *p = sni_string; char *end = p; - char *crt_file, *key_file, *ca_file, *crl_file, *auth_str; + char *crt_file, *key_file, *ca_file, *auth_str; +#if defined(MBEDTLS_X509_CRL_PARSE_C) + char *crl_file; +#endif while( *end != '\0' ) ++end; @@ -659,7 +668,9 @@ sni_entry *sni_parse( char *sni_string ) GET_ITEM( crt_file ); GET_ITEM( key_file ); GET_ITEM( ca_file ); +#if defined(MBEDTLS_X509_CRL_PARSE_C) GET_ITEM( crl_file ); +#endif GET_ITEM( auth_str ); if( ( new->cert = mbedtls_calloc( 1, sizeof( mbedtls_x509_crt ) ) ) == NULL || @@ -684,6 +695,7 @@ sni_entry *sni_parse( char *sni_string ) goto error; } +#if defined(MBEDTLS_X509_CRL_PARSE_C) if( strcmp( crl_file, "-" ) != 0 ) { if( ( new->crl = mbedtls_calloc( 1, sizeof( mbedtls_x509_crl ) ) ) == NULL ) @@ -694,6 +706,7 @@ sni_entry *sni_parse( char *sni_string ) if( mbedtls_x509_crl_parse_file( new->crl, crl_file ) != 0 ) goto error; } +#endif if( strcmp( auth_str, "-" ) != 0 ) { From 4c4ee7e0aeebc8078c235b2695552a3c82719a63 Mon Sep 17 00:00:00 2001 From: Qixiang Xu Date: Thu, 21 Feb 2019 14:55:13 +0800 Subject: [PATCH 02/32] Fix CMake build error on Cygwin and minGW platforms Signed-off-by: Qixiang Xu --- CMakeLists.txt | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 99bf31f1f..372bd85cd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -87,8 +87,14 @@ set(CMAKE_BUILD_TYPE ${CMAKE_BUILD_TYPE} # to the corresponding path in the source directory. function(link_to_source base_name) # Get OS dependent path to use in `execute_process` - file(TO_NATIVE_PATH "${CMAKE_CURRENT_BINARY_DIR}/${base_name}" link) - file(TO_NATIVE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/${base_name}" target) + if (CMAKE_HOST_WIN32) + #mklink is an internal command of cmd.exe it can only work with \ + string(REPLACE "/" "\\" link "${CMAKE_CURRENT_BINARY_DIR}/${base_name}") + string(REPLACE "/" "\\" target "${CMAKE_CURRENT_SOURCE_DIR}/${base_name}") + else() + set(link "${CMAKE_CURRENT_BINARY_DIR}/${base_name}") + set(target "${CMAKE_CURRENT_SOURCE_DIR}/${base_name}") + endif() if (NOT EXISTS ${link}) if (CMAKE_HOST_UNIX) From 9ef6028da0c73ff0186ef107870f73d95370da9d Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 2 Nov 2018 16:22:37 +0000 Subject: [PATCH 03/32] abi_check: Allow checking current checkout Without a "--detach" option, git worktree will refuse to checkout a branch that's already checked out. This makes the abi_check.py script not very useful for checking the currently checked out branch, as git will error that the branch is already checked out. Add the "--detach" option to check out the new temporary worktree in detached head mode. This is acceptable because we aren't planning on working on the branch and just want a checkout to do ABI checking from. --- scripts/abi_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 2a90b68f7..d458d56e3 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -76,7 +76,7 @@ class AbiChecker(object): ) git_worktree_path = tempfile.mkdtemp() worktree_process = subprocess.Popen( - [self.git_command, "worktree", "add", git_worktree_path, git_rev], + [self.git_command, "worktree", "add", "--detach", git_worktree_path, git_rev], cwd=self.repo_path, stdout=subprocess.PIPE, stderr=subprocess.STDOUT From 4cd4b4bf835bab6145b6865ab1c54ff21ac027dd Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Fri, 2 Nov 2018 16:35:09 +0000 Subject: [PATCH 04/32] abi_check: Update submodules When grabbing a fresh copy of a branch, it's required to also fetch the submodule. Add fetching the submodule to abi_check.py. --- scripts/abi_check.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index d458d56e3..7926bc8cd 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -87,6 +87,18 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path + def update_git_submodules(self, git_worktree_path): + process = subprocess.Popen( + [self.git_command, "submodule", "update", "--init", '--recursive'], + cwd=git_worktree_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + output, _ = process.communicate() + self.log.info(output.decode("utf-8")) + if process.returncode != 0: + raise Exception("git submodule update failed, aborting") + def build_shared_libraries(self, git_worktree_path): """Build the shared libraries in the specified worktree.""" my_environment = os.environ.copy() @@ -149,6 +161,7 @@ class AbiChecker(object): def get_abi_dump_for_ref(self, git_rev): """Generate the ABI dumps for the specified git revision.""" git_worktree_path = self.get_clean_worktree_for_git_revision(git_rev) + self.update_git_submodules(git_worktree_path) self.build_shared_libraries(git_worktree_path) abi_dumps = self.get_abi_dumps_from_shared_libraries( git_rev, git_worktree_path From 5a301f08689c6a6fc1eb46ac8a1e7f5119a9cb0c Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 19 Feb 2019 16:59:33 +0000 Subject: [PATCH 05/32] Extend abi-checking to different repos --- scripts/abi_check.py | 79 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 63 insertions(+), 16 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 7926bc8cd..d14236ead 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -28,11 +28,14 @@ import tempfile class AbiChecker(object): """API and ABI checker.""" - def __init__(self, report_dir, old_rev, new_rev, keep_all_reports): + def __init__(self, report_dir, old_repo, old_rev, new_repo, new_rev, + keep_all_reports): """Instantiate the API/ABI checker. report_dir: directory for output files + old_repo: repository for git revision to compare against old_rev: reference git revision to compare against + new_repo: repository for git revision to check new_rev: git revision to check keep_all_reports: if false, delete old reports """ @@ -42,7 +45,9 @@ class AbiChecker(object): self.report_dir = os.path.abspath(report_dir) self.keep_all_reports = keep_all_reports self.should_keep_report_dir = os.path.isdir(self.report_dir) + self.old_repo = old_repo self.old_rev = old_rev + self.new_repo = new_repo self.new_rev = new_rev self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] self.old_dumps = {} @@ -68,15 +73,35 @@ class AbiChecker(object): if not shutil.which(command): raise Exception("{} not installed, aborting".format(command)) - def get_clean_worktree_for_git_revision(self, git_rev): + def get_clean_worktree_for_git_revision(self, remote_repo, git_rev): """Make a separate worktree with git_rev checked out. Do not modify the current worktree.""" - self.log.info( - "Checking out git worktree for revision {}".format(git_rev) - ) git_worktree_path = tempfile.mkdtemp() + if remote_repo: + self.log.info( + "Checking out git worktree for revision {} from {}".format( + git_rev, remote_repo + ) + ) + fetch_process = subprocess.Popen( + [self.git_command, "fetch", remote_repo, git_rev], + cwd=self.repo_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + fetch_output, _ = fetch_process.communicate() + self.log.info(fetch_output.decode("utf-8")) + if fetch_process.returncode != 0: + raise Exception("Fetching revision failed, aborting") + worktree_rev = "FETCH_HEAD" + else: + self.log.info( + "Checking out git worktree for revision {}".format(git_rev) + ) + worktree_rev = git_rev worktree_process = subprocess.Popen( - [self.git_command, "worktree", "add", "--detach", git_worktree_path, git_rev], + [self.git_command, "worktree", "add", "--detach", + git_worktree_path, worktree_rev], cwd=self.repo_path, stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -158,9 +183,11 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, git_rev): + def get_abi_dump_for_ref(self, remote_repo, git_rev): """Generate the ABI dumps for the specified git revision.""" - git_worktree_path = self.get_clean_worktree_for_git_revision(git_rev) + git_worktree_path = self.get_clean_worktree_for_git_revision( + remote_repo, git_rev + ) self.update_git_submodules(git_worktree_path) self.build_shared_libraries(git_worktree_path) abi_dumps = self.get_abi_dumps_from_shared_libraries( @@ -226,8 +253,8 @@ class AbiChecker(object): between self.old_rev and self.new_rev.""" self.check_repo_path() self.check_abi_tools_are_installed() - self.old_dumps = self.get_abi_dump_for_ref(self.old_rev) - self.new_dumps = self.get_abi_dump_for_ref(self.new_rev) + self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev) + self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev) return self.get_abi_compatibility_report() @@ -254,17 +281,37 @@ def run_main(): help="keep all reports, even if there are no compatibility issues", ) parser.add_argument( - "-o", "--old-rev", type=str, help="revision for old version", - required=True + "-o", "--old-rev", type=str, + help=("revision for old version." + "Can include repository before revision"), + required=True, nargs="+" ) parser.add_argument( - "-n", "--new-rev", type=str, help="revision for new version", - required=True + "-n", "--new-rev", type=str, + help=("revision for new version" + "Can include repository before revision"), + required=True, nargs="+" ) abi_args = parser.parse_args() + if len(abi_args.old_rev) == 1: + old_repo = None + old_rev = abi_args.old_rev[0] + elif len(abi_args.old_rev) == 2: + old_repo = abi_args.old_rev[0] + old_rev = abi_args.old_rev[1] + else: + raise Exception("Too many arguments passed for old version") + if len(abi_args.new_rev) == 1: + new_repo = None + new_rev = abi_args.new_rev[0] + elif len(abi_args.new_rev) == 2: + new_repo = abi_args.new_rev[0] + new_rev = abi_args.new_rev[1] + else: + raise Exception("Too many arguments passed for new version") abi_check = AbiChecker( - abi_args.report_dir, abi_args.old_rev, - abi_args.new_rev, abi_args.keep_all_reports + abi_args.report_dir, old_repo, old_rev, + new_repo, new_rev, abi_args.keep_all_reports ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 668063bca2884e5e2f010eec65fc8294e7e1cd32 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 20 Feb 2019 15:01:56 +0000 Subject: [PATCH 06/32] Add option to skip identifiers in ABI checks By default abi-compliance-checker will check the entire ABI/API. There are internal identifiers that we do not promise compatibility for, so we want the ability to skip them when checking the ABI/API. --- scripts/abi_check.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index d14236ead..559501dee 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -29,7 +29,7 @@ class AbiChecker(object): """API and ABI checker.""" def __init__(self, report_dir, old_repo, old_rev, new_repo, new_rev, - keep_all_reports): + keep_all_reports, skip_file=None): """Instantiate the API/ABI checker. report_dir: directory for output files @@ -38,6 +38,7 @@ class AbiChecker(object): new_repo: repository for git revision to check new_rev: git revision to check keep_all_reports: if false, delete old reports + skip_file: path to file containing symbols and types to skip """ self.repo_path = "." self.log = None @@ -49,6 +50,7 @@ class AbiChecker(object): self.old_rev = old_rev self.new_repo = new_repo self.new_rev = new_rev + self.skip_file = skip_file self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] self.old_dumps = {} self.new_dumps = {} @@ -216,6 +218,9 @@ class AbiChecker(object): "-strict", "-report-path", output_path ] + if self.skip_file: + abi_compliance_command += ["-skip-symbols", self.skip_file, + "-skip-types", self.skip_file] abi_compliance_process = subprocess.Popen( abi_compliance_command, stdout=subprocess.PIPE, @@ -292,6 +297,10 @@ def run_main(): "Can include repository before revision"), required=True, nargs="+" ) + parser.add_argument( + "-s", "--skip-file", type=str, + help="path to file containing symbols and types to skip" + ) abi_args = parser.parse_args() if len(abi_args.old_rev) == 1: old_repo = None @@ -311,7 +320,8 @@ def run_main(): raise Exception("Too many arguments passed for new version") abi_check = AbiChecker( abi_args.report_dir, old_repo, old_rev, - new_repo, new_rev, abi_args.keep_all_reports + new_repo, new_rev, abi_args.keep_all_reports, + abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 0da4578faecb73efb7c028cd827afdfedac997c7 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 21 Feb 2019 13:09:26 +0000 Subject: [PATCH 07/32] Add option for a brief report of problems only --- scripts/abi_check.py | 71 +++++++++++++++++++++++++++++++++----------- 1 file changed, 54 insertions(+), 17 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 559501dee..afc2afe1c 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -9,10 +9,10 @@ Purpose This script is a small wrapper around the abi-compliance-checker and abi-dumper tools, applying them to compare the ABI and API of the library files from two different Git revisions within an Mbed TLS repository. -The results of the comparison are formatted as HTML and stored at -a configurable location. Returns 0 on success, 1 on ABI/API non-compliance, -and 2 if there is an error while running the script. -Note: must be run from Mbed TLS root. +The results of the comparison are either formatted as HTML and stored at +a configurable location, or a brief list of problems found is output. +Returns 0 on success, 1 on ABI/API non-compliance, and 2 if there is an error +while running the script. Note: must be run from Mbed TLS root. """ import os @@ -24,12 +24,14 @@ import argparse import logging import tempfile +import xml.etree.ElementTree as ET + class AbiChecker(object): """API and ABI checker.""" def __init__(self, report_dir, old_repo, old_rev, new_repo, new_rev, - keep_all_reports, skip_file=None): + keep_all_reports, brief, skip_file=None): """Instantiate the API/ABI checker. report_dir: directory for output files @@ -38,6 +40,7 @@ class AbiChecker(object): new_repo: repository for git revision to check new_rev: git revision to check keep_all_reports: if false, delete old reports + brief: if true, output shorter report to stdout skip_file: path to file containing symbols and types to skip """ self.repo_path = "." @@ -51,6 +54,7 @@ class AbiChecker(object): self.new_repo = new_repo self.new_rev = new_rev self.skip_file = skip_file + self.brief = brief self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] self.old_dumps = {} self.new_dumps = {} @@ -198,6 +202,24 @@ class AbiChecker(object): self.cleanup_worktree(git_worktree_path) return abi_dumps + def remove_children_with_tag(self, parent, tag): + children = parent.getchildren() + for child in children: + if child.tag == tag: + parent.remove(child) + else: + self.remove_children_with_tag(child, tag) + + def remove_extra_detail_from_report(self, report_root): + for tag in ['test_info', 'test_results', 'problem_summary', + 'added_symbols', 'removed_symbols', 'affected']: + self.remove_children_with_tag(report_root, tag) + + for report in report_root: + for problems in report.getchildren()[:]: + if not problems.getchildren(): + report.remove(problems) + def get_abi_compatibility_report(self): """Generate a report of the differences between the reference ABI and the new ABI. ABI dumps from self.old_rev and self.new_rev must @@ -216,31 +238,41 @@ class AbiChecker(object): "-old", self.old_dumps[mbed_module], "-new", self.new_dumps[mbed_module], "-strict", - "-report-path", output_path + "-report-path", output_path, ] if self.skip_file: abi_compliance_command += ["-skip-symbols", self.skip_file, "-skip-types", self.skip_file] + if self.brief: + abi_compliance_command += ["-report-format", "xml", + "-stdout"] abi_compliance_process = subprocess.Popen( abi_compliance_command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) abi_compliance_output, _ = abi_compliance_process.communicate() - self.log.info(abi_compliance_output.decode("utf-8")) if abi_compliance_process.returncode == 0: compatibility_report += ( "No compatibility issues for {}\n".format(mbed_module) ) - if not self.keep_all_reports: + if not (self.keep_all_reports or self.brief): os.remove(output_path) elif abi_compliance_process.returncode == 1: - compliance_return_code = 1 - self.should_keep_report_dir = True - compatibility_report += ( - "Compatibility issues found for {}, " - "for details see {}\n".format(mbed_module, output_path) - ) + if self.brief: + self.log.info( + "Compatibility issues found for {}".format(mbed_module) + ) + report_root = ET.fromstring(abi_compliance_output.decode("utf-8")) + self.remove_extra_detail_from_report(report_root) + self.log.info(ET.tostring(report_root).decode("utf-8")) + else: + compliance_return_code = 1 + self.can_remove_report_dir = False + compatibility_report += ( + "Compatibility issues found for {}, " + "for details see {}\n".format(mbed_module, output_path) + ) else: raise Exception( "abi-compliance-checker failed with a return code of {}," @@ -271,8 +303,9 @@ def run_main(): abi-compliance-checker and abi-dumper tools, applying them to compare the ABI and API of the library files from two different Git revisions within an Mbed TLS repository. - The results of the comparison are formatted as HTML and stored - at a configurable location. Returns 0 on success, 1 on ABI/API + The results of the comparison are either formatted as HTML and + stored at a configurable location, or a brief list of problems + found is output. Returns 0 on success, 1 on ABI/API non-compliance, and 2 if there is an error while running the script. Note: must be run from Mbed TLS root.""" ) @@ -301,6 +334,10 @@ def run_main(): "-s", "--skip-file", type=str, help="path to file containing symbols and types to skip" ) + parser.add_argument( + "-b", "--brief", action="store_true", + help="output only the list of issues to stdout, instead of a full report", + ) abi_args = parser.parse_args() if len(abi_args.old_rev) == 1: old_repo = None @@ -321,7 +358,7 @@ def run_main(): abi_check = AbiChecker( abi_args.report_dir, old_repo, old_rev, new_repo, new_rev, abi_args.keep_all_reports, - abi_args.skip_file + abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 131e24b1b56818907595575922ab1f1c4ab92463 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 25 Feb 2019 17:01:55 +0000 Subject: [PATCH 08/32] Simplify logic for checking if report folder can be removed --- scripts/abi_check.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index afc2afe1c..30f38a9bb 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -48,7 +48,8 @@ class AbiChecker(object): self.setup_logger() self.report_dir = os.path.abspath(report_dir) self.keep_all_reports = keep_all_reports - self.should_keep_report_dir = os.path.isdir(self.report_dir) + self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or + keep_all_reports) self.old_repo = old_repo self.old_rev = old_rev self.new_repo = new_repo @@ -280,7 +281,7 @@ class AbiChecker(object): ) os.remove(self.old_dumps[mbed_module]) os.remove(self.new_dumps[mbed_module]) - if not self.should_keep_report_dir and not self.keep_all_reports: + if self.can_remove_report_dir: os.rmdir(self.report_dir) self.log.info(compatibility_report) return compliance_return_code From ae5d66c61209fc1a258617446845861da037e4c0 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 25 Feb 2019 11:35:05 +0000 Subject: [PATCH 09/32] Extend functionality to allow setting crypto submodule version As going forward we will have Crypto in a submodule, we will need to be able to check ABI compatibility between versions using different submodule versions. For TLS versions that support the submodule, we will always build using the submodule. If the Crypto submodule is used, libmbedcrypto.so is not in the main library folder, but in crypto/library instead. Given this, the script searches for *.so files and notes their path, in order to create the dumps correctly. --- scripts/abi_check.py | 63 +++++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 15 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 30f38a9bb..6475a7e64 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -23,6 +23,7 @@ import subprocess import argparse import logging import tempfile +import fnmatch import xml.etree.ElementTree as ET @@ -30,15 +31,18 @@ import xml.etree.ElementTree as ET class AbiChecker(object): """API and ABI checker.""" - def __init__(self, report_dir, old_repo, old_rev, new_repo, new_rev, - keep_all_reports, brief, skip_file=None): + def __init__(self, report_dir, old_repo, old_rev, old_crypto_rev, + new_repo, new_rev, new_crypto_rev, keep_all_reports, brief, + skip_file=None): """Instantiate the API/ABI checker. report_dir: directory for output files old_repo: repository for git revision to compare against old_rev: reference git revision to compare against + old_crypto_rev: reference git revision for old crypto submodule new_repo: repository for git revision to check new_rev: git revision to check + new_crypto_rev: reference git revision for new crypto submodule keep_all_reports: if false, delete old reports brief: if true, output shorter report to stdout skip_file: path to file containing symbols and types to skip @@ -52,11 +56,13 @@ class AbiChecker(object): keep_all_reports) self.old_repo = old_repo self.old_rev = old_rev + self.old_crypto_rev = old_crypto_rev self.new_repo = new_repo self.new_rev = new_rev + self.new_crypto_rev = new_crypto_rev self.skip_file = skip_file self.brief = brief - self.mbedtls_modules = ["libmbedcrypto", "libmbedtls", "libmbedx509"] + self.mbedtls_modules = {} self.old_dumps = {} self.new_dumps = {} self.git_command = "git" @@ -119,7 +125,7 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path - def update_git_submodules(self, git_worktree_path): + def update_git_submodules(self, git_worktree_path, crypto_rev): process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -130,12 +136,25 @@ class AbiChecker(object): self.log.info(output.decode("utf-8")) if process.returncode != 0: raise Exception("git submodule update failed, aborting") + if (os.path.exists(os.path.join(git_worktree_path, "crypto")) + and crypto_rev): + checkout_process = subprocess.Popen( + [self.git_command, "checkout", crypto_rev], + cwd=os.path.join(git_worktree_path, "crypto"), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + checkout_output, _ = checkout_process.communicate() + self.log.info(checkout_output.decode("utf-8")) + if checkout_process.returncode != 0: + raise Exception("git checkout failed, aborting") def build_shared_libraries(self, git_worktree_path): """Build the shared libraries in the specified worktree.""" my_environment = os.environ.copy() my_environment["CFLAGS"] = "-g -Og" my_environment["SHARED"] = "1" + my_environment["USE_CRYPTO_SUBMODULE"] = "1" make_process = subprocess.Popen( self.make_command, env=my_environment, @@ -145,6 +164,11 @@ class AbiChecker(object): ) make_output, _ = make_process.communicate() self.log.info(make_output.decode("utf-8")) + for root, dirs, files in os.walk(git_worktree_path): + for file in fnmatch.filter(files, "*.so"): + self.mbedtls_modules[os.path.splitext(file)[0]] = os.path.join( + root, file + ) if make_process.returncode != 0: raise Exception("make failed, aborting") @@ -153,14 +177,13 @@ class AbiChecker(object): It must be checked out in git_worktree_path and the shared libraries must have been built.""" abi_dumps = {} - for mbed_module in self.mbedtls_modules: + for mbed_module, module_path in self.mbedtls_modules.items(): output_path = os.path.join( self.report_dir, "{}-{}.dump".format(mbed_module, git_ref) ) abi_dump_command = [ "abi-dumper", - os.path.join( - git_worktree_path, "library", mbed_module + ".so"), + module_path, "-o", output_path, "-lver", git_ref ] @@ -190,12 +213,12 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, remote_repo, git_rev): + def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_rev): """Generate the ABI dumps for the specified git revision.""" git_worktree_path = self.get_clean_worktree_for_git_revision( remote_repo, git_rev ) - self.update_git_submodules(git_worktree_path) + self.update_git_submodules(git_worktree_path, crypto_rev) self.build_shared_libraries(git_worktree_path) abi_dumps = self.get_abi_dumps_from_shared_libraries( git_rev, git_worktree_path @@ -227,7 +250,7 @@ class AbiChecker(object): be available.""" compatibility_report = "" compliance_return_code = 0 - for mbed_module in self.mbedtls_modules: + for mbed_module, module_path in self.mbedtls_modules.items(): output_path = os.path.join( self.report_dir, "{}-{}-{}.html".format( mbed_module, self.old_rev, self.new_rev @@ -291,8 +314,10 @@ class AbiChecker(object): between self.old_rev and self.new_rev.""" self.check_repo_path() self.check_abi_tools_are_installed() - self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev) - self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev) + self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev, + self.old_crypto_rev) + self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev, + self.new_crypto_rev) return self.get_abi_compatibility_report() @@ -325,12 +350,20 @@ def run_main(): "Can include repository before revision"), required=True, nargs="+" ) + parser.add_argument( + "-oc", "--old-crypto-rev", type=str, + help="revision for old crypto version", + ) parser.add_argument( "-n", "--new-rev", type=str, help=("revision for new version" "Can include repository before revision"), required=True, nargs="+" ) + parser.add_argument( + "-nc", "--new-crypto-rev", type=str, + help="revision for new crypto version", + ) parser.add_argument( "-s", "--skip-file", type=str, help="path to file containing symbols and types to skip" @@ -357,9 +390,9 @@ def run_main(): else: raise Exception("Too many arguments passed for new version") abi_check = AbiChecker( - abi_args.report_dir, old_repo, old_rev, - new_repo, new_rev, abi_args.keep_all_reports, - abi_args.brief, abi_args.skip_file + abi_args.report_dir, old_repo, old_rev, abi_args.old_crypto_rev, + new_repo, new_rev, abi_args.new_crypto_rev, + abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From de118091f21722933466c38ffe894d8346778963 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 27 Feb 2019 16:53:40 +0000 Subject: [PATCH 10/32] Add handling for cases when not all .so files are present We may wish to compare ABI/API between Mbed TLS and Mbed Crypto, which will cause issues as not all .so files are shared. Only compare .so files which both libraries have. --- scripts/abi_check.py | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 6475a7e64..3f697fd47 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -62,7 +62,7 @@ class AbiChecker(object): self.new_crypto_rev = new_crypto_rev self.skip_file = skip_file self.brief = brief - self.mbedtls_modules = {} + self.mbedtls_modules = {"old": {}, "new": {}} self.old_dumps = {} self.new_dumps = {} self.git_command = "git" @@ -149,7 +149,7 @@ class AbiChecker(object): if checkout_process.returncode != 0: raise Exception("git checkout failed, aborting") - def build_shared_libraries(self, git_worktree_path): + def build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" my_environment = os.environ.copy() my_environment["CFLAGS"] = "-g -Og" @@ -166,20 +166,23 @@ class AbiChecker(object): self.log.info(make_output.decode("utf-8")) for root, dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): - self.mbedtls_modules[os.path.splitext(file)[0]] = os.path.join( - root, file + self.mbedtls_modules[version][os.path.splitext(file)[0]] = ( + os.path.join(root, file) ) if make_process.returncode != 0: raise Exception("make failed, aborting") - def get_abi_dumps_from_shared_libraries(self, git_ref, git_worktree_path): + def get_abi_dumps_from_shared_libraries(self, git_ref, git_worktree_path, + version): """Generate the ABI dumps for the specified git revision. It must be checked out in git_worktree_path and the shared libraries must have been built.""" abi_dumps = {} - for mbed_module, module_path in self.mbedtls_modules.items(): + for mbed_module, module_path in self.mbedtls_modules[version].items(): output_path = os.path.join( - self.report_dir, "{}-{}.dump".format(mbed_module, git_ref) + self.report_dir, version, "{}-{}.dump".format( + mbed_module, git_ref + ) ) abi_dump_command = [ "abi-dumper", @@ -213,15 +216,15 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_rev): + def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_rev, version): """Generate the ABI dumps for the specified git revision.""" git_worktree_path = self.get_clean_worktree_for_git_revision( remote_repo, git_rev ) self.update_git_submodules(git_worktree_path, crypto_rev) - self.build_shared_libraries(git_worktree_path) + self.build_shared_libraries(git_worktree_path, version) abi_dumps = self.get_abi_dumps_from_shared_libraries( - git_rev, git_worktree_path + git_rev, git_worktree_path, version ) self.cleanup_worktree(git_worktree_path) return abi_dumps @@ -250,7 +253,9 @@ class AbiChecker(object): be available.""" compatibility_report = "" compliance_return_code = 0 - for mbed_module, module_path in self.mbedtls_modules.items(): + shared_modules = list(set(self.mbedtls_modules["old"].keys()) & + set(self.mbedtls_modules["new"].keys())) + for mbed_module in shared_modules: output_path = os.path.join( self.report_dir, "{}-{}-{}.html".format( mbed_module, self.old_rev, self.new_rev @@ -315,9 +320,9 @@ class AbiChecker(object): self.check_repo_path() self.check_abi_tools_are_installed() self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev, - self.old_crypto_rev) + self.old_crypto_rev, "old") self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev, - self.new_crypto_rev) + self.new_crypto_rev, "new") return self.get_abi_compatibility_report() From 879f2509dcb7169e6a5da89553b9fc97699372f8 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 27 Feb 2019 17:33:31 +0000 Subject: [PATCH 11/32] Add ability to compare submodules from different repositories As before with wanting to compare revisions across different repositories, the ability to select the crypto submodule from a different repository is useful. --- scripts/abi_check.py | 86 +++++++++++++++++++++++++++++++++----------- 1 file changed, 65 insertions(+), 21 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 3f697fd47..30baadcdb 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -32,17 +32,19 @@ class AbiChecker(object): """API and ABI checker.""" def __init__(self, report_dir, old_repo, old_rev, old_crypto_rev, - new_repo, new_rev, new_crypto_rev, keep_all_reports, brief, - skip_file=None): + old_crypto_repo, new_repo, new_rev, new_crypto_rev, + new_crypto_repo, keep_all_reports, brief, skip_file=None): """Instantiate the API/ABI checker. report_dir: directory for output files old_repo: repository for git revision to compare against old_rev: reference git revision to compare against old_crypto_rev: reference git revision for old crypto submodule + old_crypto_repo: repository for git revision for old crypto submodule new_repo: repository for git revision to check new_rev: git revision to check new_crypto_rev: reference git revision for new crypto submodule + new_crypto_repo: repository for git revision for new crypto submodule keep_all_reports: if false, delete old reports brief: if true, output shorter report to stdout skip_file: path to file containing symbols and types to skip @@ -57,9 +59,11 @@ class AbiChecker(object): self.old_repo = old_repo self.old_rev = old_rev self.old_crypto_rev = old_crypto_rev + self.old_crypto_repo = old_crypto_repo self.new_repo = new_repo self.new_rev = new_rev self.new_crypto_rev = new_crypto_rev + self.new_crypto_repo = new_crypto_repo self.skip_file = skip_file self.brief = brief self.mbedtls_modules = {"old": {}, "new": {}} @@ -125,7 +129,8 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path - def update_git_submodules(self, git_worktree_path, crypto_rev): + def update_git_submodules(self, git_worktree_path, crypto_repo, + crypto_rev): process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -138,16 +143,30 @@ class AbiChecker(object): raise Exception("git submodule update failed, aborting") if (os.path.exists(os.path.join(git_worktree_path, "crypto")) and crypto_rev): - checkout_process = subprocess.Popen( - [self.git_command, "checkout", crypto_rev], - cwd=os.path.join(git_worktree_path, "crypto"), - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - checkout_output, _ = checkout_process.communicate() - self.log.info(checkout_output.decode("utf-8")) - if checkout_process.returncode != 0: - raise Exception("git checkout failed, aborting") + if crypto_repo: + shutil.rmtree(os.path.join(git_worktree_path, "crypto")) + clone_process = subprocess.Popen( + [self.git_command, "clone", crypto_repo, + "--branch", crypto_rev, "crypto"], + cwd=git_worktree_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + clone_output, _ = clone_process.communicate() + self.log.info(clone_output.decode("utf-8")) + if clone_process.returncode != 0: + raise Exception("git clone failed, aborting") + else: + checkout_process = subprocess.Popen( + [self.git_command, "checkout", crypto_rev], + cwd=os.path.join(git_worktree_path, "crypto"), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + checkout_output, _ = checkout_process.communicate() + self.log.info(checkout_output.decode("utf-8")) + if checkout_process.returncode != 0: + raise Exception("git checkout failed, aborting") def build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" @@ -216,12 +235,13 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_rev, version): + def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_repo, + crypto_rev, version): """Generate the ABI dumps for the specified git revision.""" git_worktree_path = self.get_clean_worktree_for_git_revision( remote_repo, git_rev ) - self.update_git_submodules(git_worktree_path, crypto_rev) + self.update_git_submodules(git_worktree_path, crypto_repo, crypto_rev) self.build_shared_libraries(git_worktree_path, version) abi_dumps = self.get_abi_dumps_from_shared_libraries( git_rev, git_worktree_path, version @@ -320,8 +340,10 @@ class AbiChecker(object): self.check_repo_path() self.check_abi_tools_are_installed() self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev, + self.old_crypto_repo, self.old_crypto_rev, "old") self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev, + self.new_crypto_repo, self.new_crypto_rev, "new") return self.get_abi_compatibility_report() @@ -356,8 +378,9 @@ def run_main(): required=True, nargs="+" ) parser.add_argument( - "-oc", "--old-crypto-rev", type=str, - help="revision for old crypto version", + "-oc", "--old-crypto-rev", type=str, nargs="+", + help=("revision for old crypto version." + "Can include repository before revision"), ) parser.add_argument( "-n", "--new-rev", type=str, @@ -366,8 +389,9 @@ def run_main(): required=True, nargs="+" ) parser.add_argument( - "-nc", "--new-crypto-rev", type=str, - help="revision for new crypto version", + "-nc", "--new-crypto-rev", type=str, nargs="+", + help=("revision for new crypto version" + "Can include repository before revision"), ) parser.add_argument( "-s", "--skip-file", type=str, @@ -394,9 +418,29 @@ def run_main(): new_rev = abi_args.new_rev[1] else: raise Exception("Too many arguments passed for new version") + old_crypto_repo = None + old_crypto_rev = None + if abi_args.old_crypto_rev: + if len(abi_args.old_crypto_rev) == 1: + old_crypto_rev = abi_args.old_crypto_rev[0] + elif len(abi_args.old_crypto_rev) == 2: + old_crypto_repo = abi_args.old_crypto_rev[0] + old_crypto_rev = abi_args.old_crypto_rev[1] + else: + raise Exception("Too many arguments passed for old crypto version") + new_crypto_repo = None + new_crypto_rev = None + if abi_args.new_crypto_rev: + if len(abi_args.new_crypto_rev) == 1: + new_crypto_rev = abi_args.new_crypto_rev[0] + elif len(abi_args.new_crypto_rev) == 2: + new_crypto_repo = abi_args.new_crypto_rev[0] + new_crypto_rev = abi_args.new_crypto_rev[1] + else: + raise Exception("Too many arguments passed for new crypto version") abi_check = AbiChecker( - abi_args.report_dir, old_repo, old_rev, abi_args.old_crypto_rev, - new_repo, new_rev, abi_args.new_crypto_rev, + abi_args.report_dir, old_repo, old_rev, old_crypto_rev, + old_crypto_repo, new_repo, new_rev, new_crypto_rev, new_crypto_repo, abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() From c8e6ad4ace4fab318f1c2bd98fe220265c73f9ed Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 28 Feb 2019 11:52:39 +0000 Subject: [PATCH 12/32] Only build the library We only need the .so files, so only build the library --- scripts/abi_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 30baadcdb..6c8be0d68 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -175,7 +175,7 @@ class AbiChecker(object): my_environment["SHARED"] = "1" my_environment["USE_CRYPTO_SUBMODULE"] = "1" make_process = subprocess.Popen( - self.make_command, + [self.make_command, "lib"], env=my_environment, cwd=git_worktree_path, stdout=subprocess.PIPE, From 06c51d047032a6c695a1eb25fce2d7c89266b83a Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 1 Mar 2019 09:54:44 +0000 Subject: [PATCH 13/32] Use optional arguments for setting repositories --- scripts/abi_check.py | 80 +++++++++++++++----------------------------- 1 file changed, 27 insertions(+), 53 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 6c8be0d68..12ee44ed2 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -372,26 +372,34 @@ def run_main(): help="keep all reports, even if there are no compatibility issues", ) parser.add_argument( - "-o", "--old-rev", type=str, - help=("revision for old version." - "Can include repository before revision"), - required=True, nargs="+" + "-o", "--old-rev", type=str, help="revision for old version.", + required=True, ) parser.add_argument( - "-oc", "--old-crypto-rev", type=str, nargs="+", - help=("revision for old crypto version." - "Can include repository before revision"), + "-or", "--old-repo", type=str, help="repository for old version." ) parser.add_argument( - "-n", "--new-rev", type=str, - help=("revision for new version" - "Can include repository before revision"), - required=True, nargs="+" + "-oc", "--old-crypto-rev", type=str, + help="revision for old crypto submodule." ) parser.add_argument( - "-nc", "--new-crypto-rev", type=str, nargs="+", - help=("revision for new crypto version" - "Can include repository before revision"), + "-ocr", "--old-crypto-repo", type=str, + help="repository for old crypto submodule." + ) + parser.add_argument( + "-n", "--new-rev", type=str, help="revision for new version", + required=True, + ) + parser.add_argument( + "-nr", "--new-repo", type=str, help="repository for new version." + ) + parser.add_argument( + "-nc", "--new-crypto-rev", type=str, + help="revision for new crypto version" + ) + parser.add_argument( + "-ncr", "--new-crypto-repo", type=str, + help="repository for new crypto submodule." ) parser.add_argument( "-s", "--skip-file", type=str, @@ -402,46 +410,12 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() - if len(abi_args.old_rev) == 1: - old_repo = None - old_rev = abi_args.old_rev[0] - elif len(abi_args.old_rev) == 2: - old_repo = abi_args.old_rev[0] - old_rev = abi_args.old_rev[1] - else: - raise Exception("Too many arguments passed for old version") - if len(abi_args.new_rev) == 1: - new_repo = None - new_rev = abi_args.new_rev[0] - elif len(abi_args.new_rev) == 2: - new_repo = abi_args.new_rev[0] - new_rev = abi_args.new_rev[1] - else: - raise Exception("Too many arguments passed for new version") - old_crypto_repo = None - old_crypto_rev = None - if abi_args.old_crypto_rev: - if len(abi_args.old_crypto_rev) == 1: - old_crypto_rev = abi_args.old_crypto_rev[0] - elif len(abi_args.old_crypto_rev) == 2: - old_crypto_repo = abi_args.old_crypto_rev[0] - old_crypto_rev = abi_args.old_crypto_rev[1] - else: - raise Exception("Too many arguments passed for old crypto version") - new_crypto_repo = None - new_crypto_rev = None - if abi_args.new_crypto_rev: - if len(abi_args.new_crypto_rev) == 1: - new_crypto_rev = abi_args.new_crypto_rev[0] - elif len(abi_args.new_crypto_rev) == 2: - new_crypto_repo = abi_args.new_crypto_rev[0] - new_crypto_rev = abi_args.new_crypto_rev[1] - else: - raise Exception("Too many arguments passed for new crypto version") abi_check = AbiChecker( - abi_args.report_dir, old_repo, old_rev, old_crypto_rev, - old_crypto_repo, new_repo, new_rev, new_crypto_rev, new_crypto_repo, - abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file + abi_args.report_dir, abi_args.old_repo, abi_args.old_rev, + abi_args.old_crypto_rev, abi_args.old_crypto_repo, + abi_args.new_repo, abi_args.new_rev, abi_args.new_crypto_rev, + abi_args.new_crypto_repo, abi_args.keep_all_reports, + abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 7c0e0522760fd7e40c848993ea88f8a346c01d6e Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 15:21:32 +0000 Subject: [PATCH 14/32] Improve documentation --- scripts/abi_check.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 12ee44ed2..7e6d7fcfd 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -10,7 +10,7 @@ This script is a small wrapper around the abi-compliance-checker and abi-dumper tools, applying them to compare the ABI and API of the library files from two different Git revisions within an Mbed TLS repository. The results of the comparison are either formatted as HTML and stored at -a configurable location, or a brief list of problems found is output. +a configurable location, or are given as a brief list of problems. Returns 0 on success, 1 on ABI/API non-compliance, and 2 if there is an error while running the script. Note: must be run from Mbed TLS root. """ @@ -357,10 +357,10 @@ def run_main(): to compare the ABI and API of the library files from two different Git revisions within an Mbed TLS repository. The results of the comparison are either formatted as HTML and - stored at a configurable location, or a brief list of problems - found is output. Returns 0 on success, 1 on ABI/API - non-compliance, and 2 if there is an error while running the - script. Note: must be run from Mbed TLS root.""" + stored at a configurable location, or are given as a brief list + of problems. Returns 0 on success, 1 on ABI/API non-compliance, + and 2 if there is an error while running the script. + Note: must be run from Mbed TLS root.""" ) ) parser.add_argument( From 0478a321622207650dac700fa8b4df85391c0a92 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 15:23:25 +0000 Subject: [PATCH 15/32] Reduce indentation levels --- scripts/abi_check.py | 52 +++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 7e6d7fcfd..bab2fcdac 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -141,32 +141,34 @@ class AbiChecker(object): self.log.info(output.decode("utf-8")) if process.returncode != 0: raise Exception("git submodule update failed, aborting") - if (os.path.exists(os.path.join(git_worktree_path, "crypto")) + if not (os.path.exists(os.path.join(git_worktree_path, "crypto")) and crypto_rev): - if crypto_repo: - shutil.rmtree(os.path.join(git_worktree_path, "crypto")) - clone_process = subprocess.Popen( - [self.git_command, "clone", crypto_repo, - "--branch", crypto_rev, "crypto"], - cwd=git_worktree_path, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - clone_output, _ = clone_process.communicate() - self.log.info(clone_output.decode("utf-8")) - if clone_process.returncode != 0: - raise Exception("git clone failed, aborting") - else: - checkout_process = subprocess.Popen( - [self.git_command, "checkout", crypto_rev], - cwd=os.path.join(git_worktree_path, "crypto"), - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - checkout_output, _ = checkout_process.communicate() - self.log.info(checkout_output.decode("utf-8")) - if checkout_process.returncode != 0: - raise Exception("git checkout failed, aborting") + return + + if crypto_repo: + shutil.rmtree(os.path.join(git_worktree_path, "crypto")) + clone_process = subprocess.Popen( + [self.git_command, "clone", crypto_repo, + "--branch", crypto_rev, "crypto"], + cwd=git_worktree_path, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + clone_output, _ = clone_process.communicate() + self.log.info(clone_output.decode("utf-8")) + if clone_process.returncode != 0: + raise Exception("git clone failed, aborting") + else: + checkout_process = subprocess.Popen( + [self.git_command, "checkout", crypto_rev], + cwd=os.path.join(git_worktree_path, "crypto"), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + checkout_output, _ = checkout_process.communicate() + self.log.info(checkout_output.decode("utf-8")) + if checkout_process.returncode != 0: + raise Exception("git checkout failed, aborting") def build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" From 7381bea6be0c257282f731d2b2081be81974f9d4 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 16:25:38 +0000 Subject: [PATCH 16/32] Add RepoVersion class to make handling of many arguments easier There are a number of arguments being passed around, nearly all of which are duplicated between the old and new versions. Moving these into a separate class should hopefully make it simpler to follow what is being done. --- scripts/abi_check.py | 145 +++++++++++++++++++++---------------------- 1 file changed, 70 insertions(+), 75 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index bab2fcdac..cc8667899 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -28,23 +28,37 @@ import fnmatch import xml.etree.ElementTree as ET +class RepoVersion(object): + + def __init__(self, version, repository, revision, + crypto_repository, crypto_revision): + """Class containing details for a particular revision. + + version: either 'old' or 'new' + repository: repository for git revision + revision: git revision for comparison + crypto_repository: repository for git revision of crypto submodule + crypto_revision: git revision of crypto submodule + """ + self.version = version + self.repository = repository + self.revision = revision + self.crypto_repository = crypto_repository + self.crypto_revision = crypto_revision + self.abi_dumps = {} + self.modules = {} + + class AbiChecker(object): """API and ABI checker.""" - def __init__(self, report_dir, old_repo, old_rev, old_crypto_rev, - old_crypto_repo, new_repo, new_rev, new_crypto_rev, - new_crypto_repo, keep_all_reports, brief, skip_file=None): + def __init__(self, old_version, new_version, report_dir, + keep_all_reports, brief, skip_file=None): """Instantiate the API/ABI checker. + old_version: RepoVersion containing details to compare against + new_version: RepoVersion containing details to check report_dir: directory for output files - old_repo: repository for git revision to compare against - old_rev: reference git revision to compare against - old_crypto_rev: reference git revision for old crypto submodule - old_crypto_repo: repository for git revision for old crypto submodule - new_repo: repository for git revision to check - new_rev: git revision to check - new_crypto_rev: reference git revision for new crypto submodule - new_crypto_repo: repository for git revision for new crypto submodule keep_all_reports: if false, delete old reports brief: if true, output shorter report to stdout skip_file: path to file containing symbols and types to skip @@ -56,19 +70,10 @@ class AbiChecker(object): self.keep_all_reports = keep_all_reports self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or keep_all_reports) - self.old_repo = old_repo - self.old_rev = old_rev - self.old_crypto_rev = old_crypto_rev - self.old_crypto_repo = old_crypto_repo - self.new_repo = new_repo - self.new_rev = new_rev - self.new_crypto_rev = new_crypto_rev - self.new_crypto_repo = new_crypto_repo + self.old_version = old_version + self.new_version = new_version self.skip_file = skip_file self.brief = brief - self.mbedtls_modules = {"old": {}, "new": {}} - self.old_dumps = {} - self.new_dumps = {} self.git_command = "git" self.make_command = "make" @@ -90,18 +95,19 @@ class AbiChecker(object): if not shutil.which(command): raise Exception("{} not installed, aborting".format(command)) - def get_clean_worktree_for_git_revision(self, remote_repo, git_rev): - """Make a separate worktree with git_rev checked out. + def get_clean_worktree_for_git_revision(self, version): + """Make a separate worktree with version.revision checked out. Do not modify the current worktree.""" git_worktree_path = tempfile.mkdtemp() - if remote_repo: + if version.repository: self.log.info( "Checking out git worktree for revision {} from {}".format( - git_rev, remote_repo + version.revision, version.repository ) ) fetch_process = subprocess.Popen( - [self.git_command, "fetch", remote_repo, git_rev], + [self.git_command, "fetch", + version.repository, version.revision], cwd=self.repo_path, stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -112,10 +118,10 @@ class AbiChecker(object): raise Exception("Fetching revision failed, aborting") worktree_rev = "FETCH_HEAD" else: - self.log.info( - "Checking out git worktree for revision {}".format(git_rev) - ) - worktree_rev = git_rev + self.log.info("Checking out git worktree for revision {}".format( + version.revision + )) + worktree_rev = version.revision worktree_process = subprocess.Popen( [self.git_command, "worktree", "add", "--detach", git_worktree_path, worktree_rev], @@ -129,8 +135,7 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path - def update_git_submodules(self, git_worktree_path, crypto_repo, - crypto_rev): + def update_git_submodules(self, git_worktree_path, version): process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -142,14 +147,14 @@ class AbiChecker(object): if process.returncode != 0: raise Exception("git submodule update failed, aborting") if not (os.path.exists(os.path.join(git_worktree_path, "crypto")) - and crypto_rev): + and version.crypto_revision): return - if crypto_repo: + if version.crypto_repository: shutil.rmtree(os.path.join(git_worktree_path, "crypto")) clone_process = subprocess.Popen( - [self.git_command, "clone", crypto_repo, - "--branch", crypto_rev, "crypto"], + [self.git_command, "clone", version.crypto_repository, + "--branch", version.crypto_revision, "crypto"], cwd=git_worktree_path, stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -160,7 +165,7 @@ class AbiChecker(object): raise Exception("git clone failed, aborting") else: checkout_process = subprocess.Popen( - [self.git_command, "checkout", crypto_rev], + [self.git_command, "checkout", version.crypto_revision], cwd=os.path.join(git_worktree_path, "crypto"), stdout=subprocess.PIPE, stderr=subprocess.STDOUT @@ -187,29 +192,28 @@ class AbiChecker(object): self.log.info(make_output.decode("utf-8")) for root, dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): - self.mbedtls_modules[version][os.path.splitext(file)[0]] = ( + version.modules[os.path.splitext(file)[0]] = ( os.path.join(root, file) ) if make_process.returncode != 0: raise Exception("make failed, aborting") - def get_abi_dumps_from_shared_libraries(self, git_ref, git_worktree_path, + def get_abi_dumps_from_shared_libraries(self, git_worktree_path, version): """Generate the ABI dumps for the specified git revision. It must be checked out in git_worktree_path and the shared libraries must have been built.""" - abi_dumps = {} - for mbed_module, module_path in self.mbedtls_modules[version].items(): + for mbed_module, module_path in version.modules.items(): output_path = os.path.join( - self.report_dir, version, "{}-{}.dump".format( - mbed_module, git_ref + self.report_dir, version.version, "{}-{}.dump".format( + mbed_module, version.revision ) ) abi_dump_command = [ "abi-dumper", module_path, "-o", output_path, - "-lver", git_ref + "-lver", version.revision ] abi_dump_process = subprocess.Popen( abi_dump_command, @@ -220,8 +224,7 @@ class AbiChecker(object): self.log.info(abi_dump_output.decode("utf-8")) if abi_dump_process.returncode != 0: raise Exception("abi-dumper failed, aborting") - abi_dumps[mbed_module] = output_path - return abi_dumps + version.abi_dumps[mbed_module] = output_path def cleanup_worktree(self, git_worktree_path): """Remove the specified git worktree.""" @@ -237,19 +240,13 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, remote_repo, git_rev, crypto_repo, - crypto_rev, version): + def get_abi_dump_for_ref(self, version): """Generate the ABI dumps for the specified git revision.""" - git_worktree_path = self.get_clean_worktree_for_git_revision( - remote_repo, git_rev - ) - self.update_git_submodules(git_worktree_path, crypto_repo, crypto_rev) + git_worktree_path = self.get_clean_worktree_for_git_revision(version) + self.update_git_submodules(git_worktree_path, version) self.build_shared_libraries(git_worktree_path, version) - abi_dumps = self.get_abi_dumps_from_shared_libraries( - git_rev, git_worktree_path, version - ) + self.get_abi_dumps_from_shared_libraries(git_worktree_path, version) self.cleanup_worktree(git_worktree_path) - return abi_dumps def remove_children_with_tag(self, parent, tag): children = parent.getchildren() @@ -275,19 +272,20 @@ class AbiChecker(object): be available.""" compatibility_report = "" compliance_return_code = 0 - shared_modules = list(set(self.mbedtls_modules["old"].keys()) & - set(self.mbedtls_modules["new"].keys())) + shared_modules = list(set(self.old_version.modules.keys()) & + set(self.new_version.modules.keys())) for mbed_module in shared_modules: output_path = os.path.join( self.report_dir, "{}-{}-{}.html".format( - mbed_module, self.old_rev, self.new_rev + mbed_module, self.old_version.revision, + self.new_version.revision ) ) abi_compliance_command = [ "abi-compliance-checker", "-l", mbed_module, - "-old", self.old_dumps[mbed_module], - "-new", self.new_dumps[mbed_module], + "-old", self.old_version.abi_dumps[mbed_module], + "-new", self.new_version.abi_dumps[mbed_module], "-strict", "-report-path", output_path, ] @@ -329,8 +327,8 @@ class AbiChecker(object): "abi-compliance-checker failed with a return code of {}," " aborting".format(abi_compliance_process.returncode) ) - os.remove(self.old_dumps[mbed_module]) - os.remove(self.new_dumps[mbed_module]) + os.remove(self.old_version.abi_dumps[mbed_module]) + os.remove(self.new_version.abi_dumps[mbed_module]) if self.can_remove_report_dir: os.rmdir(self.report_dir) self.log.info(compatibility_report) @@ -341,12 +339,8 @@ class AbiChecker(object): between self.old_rev and self.new_rev.""" self.check_repo_path() self.check_abi_tools_are_installed() - self.old_dumps = self.get_abi_dump_for_ref(self.old_repo, self.old_rev, - self.old_crypto_repo, - self.old_crypto_rev, "old") - self.new_dumps = self.get_abi_dump_for_ref(self.new_repo, self.new_rev, - self.new_crypto_repo, - self.new_crypto_rev, "new") + self.get_abi_dump_for_ref(self.old_version) + self.get_abi_dump_for_ref(self.new_version) return self.get_abi_compatibility_report() @@ -412,12 +406,13 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() + old_version = RepoVersion("old", abi_args.old_repo, abi_args.old_rev, + abi_args.old_crypto_repo, abi_args.old_crypto_rev) + new_version = RepoVersion("new", abi_args.new_repo, abi_args.new_rev, + abi_args.new_crypto_repo, abi_args.new_crypto_rev) abi_check = AbiChecker( - abi_args.report_dir, abi_args.old_repo, abi_args.old_rev, - abi_args.old_crypto_rev, abi_args.old_crypto_repo, - abi_args.new_repo, abi_args.new_rev, abi_args.new_crypto_rev, - abi_args.new_crypto_repo, abi_args.keep_all_reports, - abi_args.brief, abi_args.skip_file + old_version, new_version, abi_args.report_dir, + abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) From 88bfbc2deb78847679b1385a1eecb14ce2f00d01 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 16:30:39 +0000 Subject: [PATCH 17/32] Prefix internal functions with underscore --- scripts/abi_check.py | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index cc8667899..193169154 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -65,7 +65,7 @@ class AbiChecker(object): """ self.repo_path = "." self.log = None - self.setup_logger() + self._setup_logger() self.report_dir = os.path.abspath(report_dir) self.keep_all_reports = keep_all_reports self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or @@ -84,7 +84,7 @@ class AbiChecker(object): if current_dir != root_dir: raise Exception("Must be run from Mbed TLS root") - def setup_logger(self): + def _setup_logger(self): self.log = logging.getLogger() self.log.setLevel(logging.INFO) self.log.addHandler(logging.StreamHandler()) @@ -95,7 +95,7 @@ class AbiChecker(object): if not shutil.which(command): raise Exception("{} not installed, aborting".format(command)) - def get_clean_worktree_for_git_revision(self, version): + def _get_clean_worktree_for_git_revision(self, version): """Make a separate worktree with version.revision checked out. Do not modify the current worktree.""" git_worktree_path = tempfile.mkdtemp() @@ -135,7 +135,7 @@ class AbiChecker(object): raise Exception("Checking out worktree failed, aborting") return git_worktree_path - def update_git_submodules(self, git_worktree_path, version): + def _update_git_submodules(self, git_worktree_path, version): process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -175,7 +175,7 @@ class AbiChecker(object): if checkout_process.returncode != 0: raise Exception("git checkout failed, aborting") - def build_shared_libraries(self, git_worktree_path, version): + def _build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" my_environment = os.environ.copy() my_environment["CFLAGS"] = "-g -Og" @@ -198,8 +198,8 @@ class AbiChecker(object): if make_process.returncode != 0: raise Exception("make failed, aborting") - def get_abi_dumps_from_shared_libraries(self, git_worktree_path, - version): + def _get_abi_dumps_from_shared_libraries(self, git_worktree_path, + version): """Generate the ABI dumps for the specified git revision. It must be checked out in git_worktree_path and the shared libraries must have been built.""" @@ -226,7 +226,7 @@ class AbiChecker(object): raise Exception("abi-dumper failed, aborting") version.abi_dumps[mbed_module] = output_path - def cleanup_worktree(self, git_worktree_path): + def _cleanup_worktree(self, git_worktree_path): """Remove the specified git worktree.""" shutil.rmtree(git_worktree_path) worktree_process = subprocess.Popen( @@ -240,26 +240,26 @@ class AbiChecker(object): if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") - def get_abi_dump_for_ref(self, version): + def _get_abi_dump_for_ref(self, version): """Generate the ABI dumps for the specified git revision.""" - git_worktree_path = self.get_clean_worktree_for_git_revision(version) - self.update_git_submodules(git_worktree_path, version) - self.build_shared_libraries(git_worktree_path, version) - self.get_abi_dumps_from_shared_libraries(git_worktree_path, version) - self.cleanup_worktree(git_worktree_path) + git_worktree_path = self._get_clean_worktree_for_git_revision(version) + self._update_git_submodules(git_worktree_path, version) + self._build_shared_libraries(git_worktree_path, version) + self._get_abi_dumps_from_shared_libraries(git_worktree_path, version) + self._cleanup_worktree(git_worktree_path) - def remove_children_with_tag(self, parent, tag): + def _remove_children_with_tag(self, parent, tag): children = parent.getchildren() for child in children: if child.tag == tag: parent.remove(child) else: - self.remove_children_with_tag(child, tag) + self._remove_children_with_tag(child, tag) - def remove_extra_detail_from_report(self, report_root): + def _remove_extra_detail_from_report(self, report_root): for tag in ['test_info', 'test_results', 'problem_summary', 'added_symbols', 'removed_symbols', 'affected']: - self.remove_children_with_tag(report_root, tag) + self._remove_children_with_tag(report_root, tag) for report in report_root: for problems in report.getchildren()[:]: @@ -313,7 +313,7 @@ class AbiChecker(object): "Compatibility issues found for {}".format(mbed_module) ) report_root = ET.fromstring(abi_compliance_output.decode("utf-8")) - self.remove_extra_detail_from_report(report_root) + self._remove_extra_detail_from_report(report_root) self.log.info(ET.tostring(report_root).decode("utf-8")) else: compliance_return_code = 1 @@ -339,8 +339,8 @@ class AbiChecker(object): between self.old_rev and self.new_rev.""" self.check_repo_path() self.check_abi_tools_are_installed() - self.get_abi_dump_for_ref(self.old_version) - self.get_abi_dump_for_ref(self.new_version) + self._get_abi_dump_for_ref(self.old_version) + self._get_abi_dump_for_ref(self.new_version) return self.get_abi_compatibility_report() From 3f742987b86a7c9146c5980ac916a851763995fc Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 8 Mar 2019 11:12:19 +0000 Subject: [PATCH 18/32] Fetch the remote crypto branch, rather than cloning it --- scripts/abi_check.py | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 193169154..d23b038ea 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -151,29 +151,31 @@ class AbiChecker(object): return if version.crypto_repository: - shutil.rmtree(os.path.join(git_worktree_path, "crypto")) - clone_process = subprocess.Popen( - [self.git_command, "clone", version.crypto_repository, - "--branch", version.crypto_revision, "crypto"], - cwd=git_worktree_path, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - clone_output, _ = clone_process.communicate() - self.log.info(clone_output.decode("utf-8")) - if clone_process.returncode != 0: - raise Exception("git clone failed, aborting") - else: - checkout_process = subprocess.Popen( - [self.git_command, "checkout", version.crypto_revision], + fetch_process = subprocess.Popen( + [self.git_command, "fetch", version.crypto_repository, + version.crypto_revision], cwd=os.path.join(git_worktree_path, "crypto"), stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - checkout_output, _ = checkout_process.communicate() - self.log.info(checkout_output.decode("utf-8")) - if checkout_process.returncode != 0: - raise Exception("git checkout failed, aborting") + fetch_output, _ = fetch_process.communicate() + self.log.info(fetch_output.decode("utf-8")) + if fetch_process.returncode != 0: + raise Exception("git fetch failed, aborting") + crypto_rev = "FETCH_HEAD" + else: + crypto_rev = version.crypto_revision + + checkout_process = subprocess.Popen( + [self.git_command, "checkout", crypto_rev], + cwd=os.path.join(git_worktree_path, "crypto"), + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT + ) + checkout_output, _ = checkout_process.communicate() + self.log.info(checkout_output.decode("utf-8")) + if checkout_process.returncode != 0: + raise Exception("git checkout failed, aborting") def _build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" From 66025385443b93cb09e5058658db5b7632c57c13 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 8 Mar 2019 11:30:04 +0000 Subject: [PATCH 19/32] Add verbose switch to silence all output except the final report --- scripts/abi_check.py | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index d23b038ea..a25c85fc1 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -52,7 +52,7 @@ class RepoVersion(object): class AbiChecker(object): """API and ABI checker.""" - def __init__(self, old_version, new_version, report_dir, + def __init__(self, verbose, old_version, new_version, report_dir, keep_all_reports, brief, skip_file=None): """Instantiate the API/ABI checker. @@ -65,6 +65,7 @@ class AbiChecker(object): """ self.repo_path = "." self.log = None + self.verbose = verbose self._setup_logger() self.report_dir = os.path.abspath(report_dir) self.keep_all_reports = keep_all_reports @@ -86,7 +87,10 @@ class AbiChecker(object): def _setup_logger(self): self.log = logging.getLogger() - self.log.setLevel(logging.INFO) + if self.verbose: + self.log.setLevel(logging.DEBUG) + else: + self.log.setLevel(logging.INFO) self.log.addHandler(logging.StreamHandler()) @staticmethod @@ -100,7 +104,7 @@ class AbiChecker(object): Do not modify the current worktree.""" git_worktree_path = tempfile.mkdtemp() if version.repository: - self.log.info( + self.log.debug( "Checking out git worktree for revision {} from {}".format( version.revision, version.repository ) @@ -113,12 +117,12 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) fetch_output, _ = fetch_process.communicate() - self.log.info(fetch_output.decode("utf-8")) + self.log.debug(fetch_output.decode("utf-8")) if fetch_process.returncode != 0: raise Exception("Fetching revision failed, aborting") worktree_rev = "FETCH_HEAD" else: - self.log.info("Checking out git worktree for revision {}".format( + self.log.debug("Checking out git worktree for revision {}".format( version.revision )) worktree_rev = version.revision @@ -130,7 +134,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) worktree_output, _ = worktree_process.communicate() - self.log.info(worktree_output.decode("utf-8")) + self.log.debug(worktree_output.decode("utf-8")) if worktree_process.returncode != 0: raise Exception("Checking out worktree failed, aborting") return git_worktree_path @@ -143,7 +147,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) output, _ = process.communicate() - self.log.info(output.decode("utf-8")) + self.log.debug(output.decode("utf-8")) if process.returncode != 0: raise Exception("git submodule update failed, aborting") if not (os.path.exists(os.path.join(git_worktree_path, "crypto")) @@ -159,7 +163,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) fetch_output, _ = fetch_process.communicate() - self.log.info(fetch_output.decode("utf-8")) + self.log.debug(fetch_output.decode("utf-8")) if fetch_process.returncode != 0: raise Exception("git fetch failed, aborting") crypto_rev = "FETCH_HEAD" @@ -173,7 +177,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) checkout_output, _ = checkout_process.communicate() - self.log.info(checkout_output.decode("utf-8")) + self.log.debug(checkout_output.decode("utf-8")) if checkout_process.returncode != 0: raise Exception("git checkout failed, aborting") @@ -191,7 +195,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) make_output, _ = make_process.communicate() - self.log.info(make_output.decode("utf-8")) + self.log.debug(make_output.decode("utf-8")) for root, dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): version.modules[os.path.splitext(file)[0]] = ( @@ -223,7 +227,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) abi_dump_output, _ = abi_dump_process.communicate() - self.log.info(abi_dump_output.decode("utf-8")) + self.log.debug(abi_dump_output.decode("utf-8")) if abi_dump_process.returncode != 0: raise Exception("abi-dumper failed, aborting") version.abi_dumps[mbed_module] = output_path @@ -238,7 +242,7 @@ class AbiChecker(object): stderr=subprocess.STDOUT ) worktree_output, _ = worktree_process.communicate() - self.log.info(worktree_output.decode("utf-8")) + self.log.debug(worktree_output.decode("utf-8")) if worktree_process.returncode != 0: raise Exception("Worktree cleanup failed, aborting") @@ -361,6 +365,10 @@ def run_main(): Note: must be run from Mbed TLS root.""" ) ) + parser.add_argument( + "-v", "--verbose", action="store_true", + help="set verbosity level", + ) parser.add_argument( "-r", "--report-dir", type=str, default="reports", help="directory where reports are stored, default is reports", @@ -413,7 +421,7 @@ def run_main(): new_version = RepoVersion("new", abi_args.new_repo, abi_args.new_rev, abi_args.new_crypto_repo, abi_args.new_crypto_rev) abi_check = AbiChecker( - old_version, new_version, abi_args.report_dir, + abi_args.verbose, old_version, new_version, abi_args.report_dir, abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file ) return_code = abi_check.check_for_abi_changes() From 5783847c639be6c7aaf684b631a78a041adadc83 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 4 Apr 2019 14:39:33 +0100 Subject: [PATCH 20/32] Don't put abi dumps in subfolders --- scripts/abi_check.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index a25c85fc1..a5ef7e648 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -211,8 +211,8 @@ class AbiChecker(object): must have been built.""" for mbed_module, module_path in version.modules.items(): output_path = os.path.join( - self.report_dir, version.version, "{}-{}.dump".format( - mbed_module, version.revision + self.report_dir, "{}-{}-{}.dump".format( + mbed_module, version.revision, version.version ) ) abi_dump_command = [ From 26dff8e68d4029441d7857bf67dbcc0bd390ca65 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 5 Apr 2019 17:06:17 +0100 Subject: [PATCH 21/32] Fix pylint issues --- scripts/abi_check.py | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index a5ef7e648..9f99309af 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -140,6 +140,9 @@ class AbiChecker(object): return git_worktree_path def _update_git_submodules(self, git_worktree_path, version): + """If the crypto submodule is present, initialize it. + if version.crypto_revision exists, update it to that revision, + otherwise update it to the default revision""" process = subprocess.Popen( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, @@ -196,7 +199,7 @@ class AbiChecker(object): ) make_output, _ = make_process.communicate() self.log.debug(make_output.decode("utf-8")) - for root, dirs, files in os.walk(git_worktree_path): + for root, dirs, files in os.walk(git_worktree_path): # pylint: disable=unused-variable for file in fnmatch.filter(files, "*.so"): version.modules[os.path.splitext(file)[0]] = ( os.path.join(root, file) @@ -204,11 +207,10 @@ class AbiChecker(object): if make_process.returncode != 0: raise Exception("make failed, aborting") - def _get_abi_dumps_from_shared_libraries(self, git_worktree_path, - version): + def _get_abi_dumps_from_shared_libraries(self, version): """Generate the ABI dumps for the specified git revision. - It must be checked out in git_worktree_path and the shared libraries - must have been built.""" + The shared libraries must have been built and the module paths + present in version.modules.""" for mbed_module, module_path in version.modules.items(): output_path = os.path.join( self.report_dir, "{}-{}-{}.dump".format( @@ -251,7 +253,7 @@ class AbiChecker(object): git_worktree_path = self._get_clean_worktree_for_git_revision(version) self._update_git_submodules(git_worktree_path, version) self._build_shared_libraries(git_worktree_path, version) - self._get_abi_dumps_from_shared_libraries(git_worktree_path, version) + self._get_abi_dumps_from_shared_libraries(version) self._cleanup_worktree(git_worktree_path) def _remove_children_with_tag(self, parent, tag): @@ -264,7 +266,7 @@ class AbiChecker(object): def _remove_extra_detail_from_report(self, report_root): for tag in ['test_info', 'test_results', 'problem_summary', - 'added_symbols', 'removed_symbols', 'affected']: + 'added_symbols', 'removed_symbols', 'affected']: self._remove_children_with_tag(report_root, tag) for report in report_root: @@ -274,8 +276,8 @@ class AbiChecker(object): def get_abi_compatibility_report(self): """Generate a report of the differences between the reference ABI - and the new ABI. ABI dumps from self.old_rev and self.new_rev must - be available.""" + and the new ABI. ABI dumps from self.old_version and self.new_version + must be available.""" compatibility_report = "" compliance_return_code = 0 shared_modules = list(set(self.old_version.modules.keys()) & @@ -416,10 +418,14 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() - old_version = RepoVersion("old", abi_args.old_repo, abi_args.old_rev, - abi_args.old_crypto_repo, abi_args.old_crypto_rev) - new_version = RepoVersion("new", abi_args.new_repo, abi_args.new_rev, - abi_args.new_crypto_repo, abi_args.new_crypto_rev) + old_version = RepoVersion( + "old", abi_args.old_repo, abi_args.old_rev, + abi_args.old_crypto_repo, abi_args.old_crypto_rev + ) + new_version = RepoVersion( + "new", abi_args.new_repo, abi_args.new_rev, + abi_args.new_crypto_repo, abi_args.new_crypto_rev + ) abi_check = AbiChecker( abi_args.verbose, old_version, new_version, abi_args.report_dir, abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file From 30dc6d5bb9364dec520763fe6528bb13a5eb3066 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 9 Apr 2019 09:14:17 +0100 Subject: [PATCH 22/32] Use namespaces instead of full classes --- scripts/abi_check.py | 69 ++++++++++++++++++++------------------------ 1 file changed, 31 insertions(+), 38 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 9f99309af..5a92a7d40 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -24,36 +24,15 @@ import argparse import logging import tempfile import fnmatch +from types import SimpleNamespace import xml.etree.ElementTree as ET -class RepoVersion(object): - - def __init__(self, version, repository, revision, - crypto_repository, crypto_revision): - """Class containing details for a particular revision. - - version: either 'old' or 'new' - repository: repository for git revision - revision: git revision for comparison - crypto_repository: repository for git revision of crypto submodule - crypto_revision: git revision of crypto submodule - """ - self.version = version - self.repository = repository - self.revision = revision - self.crypto_repository = crypto_repository - self.crypto_revision = crypto_revision - self.abi_dumps = {} - self.modules = {} - - class AbiChecker(object): """API and ABI checker.""" - def __init__(self, verbose, old_version, new_version, report_dir, - keep_all_reports, brief, skip_file=None): + def __init__(self, old_version, new_version, configuration): """Instantiate the API/ABI checker. old_version: RepoVersion containing details to compare against @@ -65,16 +44,16 @@ class AbiChecker(object): """ self.repo_path = "." self.log = None - self.verbose = verbose + self.verbose = configuration.verbose self._setup_logger() - self.report_dir = os.path.abspath(report_dir) - self.keep_all_reports = keep_all_reports + self.report_dir = os.path.abspath(configuration.report_dir) + self.keep_all_reports = configuration.keep_all_reports self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or - keep_all_reports) + self.keep_all_reports) self.old_version = old_version self.new_version = new_version - self.skip_file = skip_file - self.brief = brief + self.skip_file = configuration.skip_file + self.brief = configuration.brief self.git_command = "git" self.make_command = "make" @@ -418,18 +397,32 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() - old_version = RepoVersion( - "old", abi_args.old_repo, abi_args.old_rev, - abi_args.old_crypto_repo, abi_args.old_crypto_rev + old_version = SimpleNamespace( + version="old", + repository=abi_args.old_repo, + revision=abi_args.old_rev, + crypto_repository=abi_args.old_crypto_repo, + crypto_revision=abi_args.old_crypto_rev, + abi_dumps={}, + modules={} ) - new_version = RepoVersion( - "new", abi_args.new_repo, abi_args.new_rev, - abi_args.new_crypto_repo, abi_args.new_crypto_rev + new_version = SimpleNamespace( + version="new", + repository=abi_args.new_repo, + revision=abi_args.new_rev, + crypto_repository=abi_args.new_crypto_repo, + crypto_revision=abi_args.new_crypto_rev, + abi_dumps={}, + modules={} ) - abi_check = AbiChecker( - abi_args.verbose, old_version, new_version, abi_args.report_dir, - abi_args.keep_all_reports, abi_args.brief, abi_args.skip_file + configuration = SimpleNamespace( + verbose=abi_args.verbose, + report_dir=abi_args.report_dir, + keep_all_reports=abi_args.keep_all_reports, + brief=abi_args.brief, + skip_file=abi_args.skip_file ) + abi_check = AbiChecker(old_version, new_version, configuration) return_code = abi_check.check_for_abi_changes() sys.exit(return_code) except Exception: # pylint: disable=broad-except From 6b3cbfa370c4025af3b349e843e4d47db8f78477 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 11 Apr 2019 15:50:41 +0100 Subject: [PATCH 23/32] Check that the report directory is a directory --- scripts/abi_check.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 5a92a7d40..e6253e99b 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -48,7 +48,7 @@ class AbiChecker(object): self._setup_logger() self.report_dir = os.path.abspath(configuration.report_dir) self.keep_all_reports = configuration.keep_all_reports - self.can_remove_report_dir = not (os.path.isdir(self.report_dir) or + self.can_remove_report_dir = not (os.path.exists(self.report_dir) or self.keep_all_reports) self.old_version = old_version self.new_version = new_version @@ -397,6 +397,9 @@ def run_main(): help="output only the list of issues to stdout, instead of a full report", ) abi_args = parser.parse_args() + if os.path.isfile(abi_args.report_dir): + print("Error: {} is not a directory".format(abi_args.report_dir)) + parser.exit() old_version = SimpleNamespace( version="old", repository=abi_args.old_repo, From 3c9e7d23b1f62785566872276110ace98931715f Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 15:17:02 +0100 Subject: [PATCH 24/32] Correct documentation --- scripts/abi_check.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index e6253e99b..0bbb9f872 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -37,10 +37,10 @@ class AbiChecker(object): old_version: RepoVersion containing details to compare against new_version: RepoVersion containing details to check - report_dir: directory for output files - keep_all_reports: if false, delete old reports - brief: if true, output shorter report to stdout - skip_file: path to file containing symbols and types to skip + configuration.report_dir: directory for output files + configuration.keep_all_reports: if false, delete old reports + configuration.brief: if true, output shorter report to stdout + configuration.skip_file: path to file containing symbols and types to skip """ self.repo_path = "." self.log = None From 03c5e856e87508891e9afa5b165fff31428b40e7 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 15:18:02 +0100 Subject: [PATCH 25/32] Start unused variable with underscore --- scripts/abi_check.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index 0bbb9f872..af293cd2a 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -178,7 +178,7 @@ class AbiChecker(object): ) make_output, _ = make_process.communicate() self.log.debug(make_output.decode("utf-8")) - for root, dirs, files in os.walk(git_worktree_path): # pylint: disable=unused-variable + for root, _dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): version.modules[os.path.splitext(file)[0]] = ( os.path.join(root, file) From a0415bc779905987607a0cf3908fa7971d07dc4d Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 16:24:25 +0100 Subject: [PATCH 26/32] Use check_output instead of Popen --- scripts/abi_check.py | 101 ++++++++++++++----------------------------- 1 file changed, 33 insertions(+), 68 deletions(-) diff --git a/scripts/abi_check.py b/scripts/abi_check.py index af293cd2a..f837f7a79 100755 --- a/scripts/abi_check.py +++ b/scripts/abi_check.py @@ -88,80 +88,60 @@ class AbiChecker(object): version.revision, version.repository ) ) - fetch_process = subprocess.Popen( + fetch_output = subprocess.check_output( [self.git_command, "fetch", version.repository, version.revision], cwd=self.repo_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - fetch_output, _ = fetch_process.communicate() self.log.debug(fetch_output.decode("utf-8")) - if fetch_process.returncode != 0: - raise Exception("Fetching revision failed, aborting") worktree_rev = "FETCH_HEAD" else: self.log.debug("Checking out git worktree for revision {}".format( version.revision )) worktree_rev = version.revision - worktree_process = subprocess.Popen( + worktree_output = subprocess.check_output( [self.git_command, "worktree", "add", "--detach", git_worktree_path, worktree_rev], cwd=self.repo_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - worktree_output, _ = worktree_process.communicate() self.log.debug(worktree_output.decode("utf-8")) - if worktree_process.returncode != 0: - raise Exception("Checking out worktree failed, aborting") return git_worktree_path def _update_git_submodules(self, git_worktree_path, version): """If the crypto submodule is present, initialize it. if version.crypto_revision exists, update it to that revision, otherwise update it to the default revision""" - process = subprocess.Popen( + update_output = subprocess.check_output( [self.git_command, "submodule", "update", "--init", '--recursive'], cwd=git_worktree_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - output, _ = process.communicate() - self.log.debug(output.decode("utf-8")) - if process.returncode != 0: - raise Exception("git submodule update failed, aborting") + self.log.debug(update_output.decode("utf-8")) if not (os.path.exists(os.path.join(git_worktree_path, "crypto")) and version.crypto_revision): return if version.crypto_repository: - fetch_process = subprocess.Popen( + fetch_output = subprocess.check_output( [self.git_command, "fetch", version.crypto_repository, version.crypto_revision], cwd=os.path.join(git_worktree_path, "crypto"), - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - fetch_output, _ = fetch_process.communicate() self.log.debug(fetch_output.decode("utf-8")) - if fetch_process.returncode != 0: - raise Exception("git fetch failed, aborting") crypto_rev = "FETCH_HEAD" else: crypto_rev = version.crypto_revision - checkout_process = subprocess.Popen( + checkout_output = subprocess.check_output( [self.git_command, "checkout", crypto_rev], cwd=os.path.join(git_worktree_path, "crypto"), - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - checkout_output, _ = checkout_process.communicate() self.log.debug(checkout_output.decode("utf-8")) - if checkout_process.returncode != 0: - raise Exception("git checkout failed, aborting") def _build_shared_libraries(self, git_worktree_path, version): """Build the shared libraries in the specified worktree.""" @@ -169,22 +149,18 @@ class AbiChecker(object): my_environment["CFLAGS"] = "-g -Og" my_environment["SHARED"] = "1" my_environment["USE_CRYPTO_SUBMODULE"] = "1" - make_process = subprocess.Popen( + make_output = subprocess.check_output( [self.make_command, "lib"], env=my_environment, cwd=git_worktree_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - make_output, _ = make_process.communicate() self.log.debug(make_output.decode("utf-8")) for root, _dirs, files in os.walk(git_worktree_path): for file in fnmatch.filter(files, "*.so"): version.modules[os.path.splitext(file)[0]] = ( os.path.join(root, file) ) - if make_process.returncode != 0: - raise Exception("make failed, aborting") def _get_abi_dumps_from_shared_libraries(self, version): """Generate the ABI dumps for the specified git revision. @@ -202,30 +178,22 @@ class AbiChecker(object): "-o", output_path, "-lver", version.revision ] - abi_dump_process = subprocess.Popen( + abi_dump_output = subprocess.check_output( abi_dump_command, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - abi_dump_output, _ = abi_dump_process.communicate() self.log.debug(abi_dump_output.decode("utf-8")) - if abi_dump_process.returncode != 0: - raise Exception("abi-dumper failed, aborting") version.abi_dumps[mbed_module] = output_path def _cleanup_worktree(self, git_worktree_path): """Remove the specified git worktree.""" shutil.rmtree(git_worktree_path) - worktree_process = subprocess.Popen( + worktree_output = subprocess.check_output( [self.git_command, "worktree", "prune"], cwd=self.repo_path, - stdout=subprocess.PIPE, stderr=subprocess.STDOUT ) - worktree_output, _ = worktree_process.communicate() self.log.debug(worktree_output.decode("utf-8")) - if worktree_process.returncode != 0: - raise Exception("Worktree cleanup failed, aborting") def _get_abi_dump_for_ref(self, version): """Generate the ABI dumps for the specified git revision.""" @@ -282,38 +250,35 @@ class AbiChecker(object): if self.brief: abi_compliance_command += ["-report-format", "xml", "-stdout"] - abi_compliance_process = subprocess.Popen( - abi_compliance_command, - stdout=subprocess.PIPE, - stderr=subprocess.STDOUT - ) - abi_compliance_output, _ = abi_compliance_process.communicate() - if abi_compliance_process.returncode == 0: + try: + subprocess.check_output( + abi_compliance_command, + stderr=subprocess.STDOUT + ) + except subprocess.CalledProcessError as err: + if err.returncode == 1: + compliance_return_code = 1 + if self.brief: + self.log.info( + "Compatibility issues found for {}".format(mbed_module) + ) + report_root = ET.fromstring(err.output.decode("utf-8")) + self._remove_extra_detail_from_report(report_root) + self.log.info(ET.tostring(report_root).decode("utf-8")) + else: + self.can_remove_report_dir = False + compatibility_report += ( + "Compatibility issues found for {}, " + "for details see {}\n".format(mbed_module, output_path) + ) + else: + raise err + else: compatibility_report += ( "No compatibility issues for {}\n".format(mbed_module) ) if not (self.keep_all_reports or self.brief): os.remove(output_path) - elif abi_compliance_process.returncode == 1: - if self.brief: - self.log.info( - "Compatibility issues found for {}".format(mbed_module) - ) - report_root = ET.fromstring(abi_compliance_output.decode("utf-8")) - self._remove_extra_detail_from_report(report_root) - self.log.info(ET.tostring(report_root).decode("utf-8")) - else: - compliance_return_code = 1 - self.can_remove_report_dir = False - compatibility_report += ( - "Compatibility issues found for {}, " - "for details see {}\n".format(mbed_module, output_path) - ) - else: - raise Exception( - "abi-compliance-checker failed with a return code of {}," - " aborting".format(abi_compliance_process.returncode) - ) os.remove(self.old_version.abi_dumps[mbed_module]) os.remove(self.new_version.abi_dumps[mbed_module]) if self.can_remove_report_dir: From 3ef06d5bf78d4db59ac02e9e329e85cbc7e6be4f Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 4 Apr 2019 11:33:22 +0100 Subject: [PATCH 27/32] Add --internal option to list-identifiers.sh When doing ABI/API checking, its useful to have a list of all the identifiers that are defined in the internal header files, as we do not promise compatibility for them. This option allows for a simple method of getting them for use with the ABI checking script. --- tests/scripts/list-identifiers.sh | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/tests/scripts/list-identifiers.sh b/tests/scripts/list-identifiers.sh index 130d9d63f..4303413c4 100755 --- a/tests/scripts/list-identifiers.sh +++ b/tests/scripts/list-identifiers.sh @@ -1,4 +1,9 @@ -#!/bin/sh +#!/bin/bash +# +# Outputs a file containing identifiers from internal header files or all +# header files, based on --internal flag. +# +# Usage: list-identifiers.sh [ -i | --internal ] set -eu @@ -7,7 +12,29 @@ if [ -d include/mbedtls ]; then :; else exit 1 fi -HEADERS=$( ls include/mbedtls/*.h | egrep -v 'compat-1\.3\.h|bn_mul' ) +INTERNAL="" + +until [ -z "${1-}" ] +do + case "$1" in + -i|--internal) + INTERNAL="1" + ;; + *) + # print error + echo "Unknown argument: '$1'" + exit 1 + ;; + esac + shift +done + +if [ $INTERNAL ] +then + HEADERS=$( ls include/mbedtls/*_internal.h | egrep -v 'compat-1\.3\.h|bn_mul' ) +else + HEADERS=$( ls include/mbedtls/*.h | egrep -v 'compat-1\.3\.h|bn_mul' ) +fi rm -f identifiers From 3997a6cd2563e886b28819461b1ed0561366d802 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 18 Apr 2019 13:09:25 +0100 Subject: [PATCH 28/32] Document the scripts behaviour further --- tests/scripts/list-identifiers.sh | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/scripts/list-identifiers.sh b/tests/scripts/list-identifiers.sh index 4303413c4..cc9c54fad 100755 --- a/tests/scripts/list-identifiers.sh +++ b/tests/scripts/list-identifiers.sh @@ -1,7 +1,8 @@ #!/bin/bash # -# Outputs a file containing identifiers from internal header files or all -# header files, based on --internal flag. +# Create a file named identifiers containing identifiers from internal header +# files or all header files, based on --internal flag. +# Outputs the line count of the file to stdout. # # Usage: list-identifiers.sh [ -i | --internal ] From d6028a1894ed9675769b13ae619a0ea6d937882b Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 15 Oct 2018 12:01:35 +0100 Subject: [PATCH 29/32] Improve macro hygiene This commit improves hygiene and formatting of macro definitions throughout the library. Specifically: - It adds brackets around parameters to avoid unintended interpretation of arguments, e.g. due to operator precedence. - It adds uses of the `do { ... } while( 0 )` idiom for macros that can be used as commands. --- include/mbedtls/asn1write.h | 7 +-- include/mbedtls/bignum.h | 7 ++- include/mbedtls/padlock.h | 2 +- include/mbedtls/x509_crt.h | 2 +- library/aes.c | 96 +++++++++++++++++++------------------ library/ccm.c | 16 +++++-- library/chacha20.c | 10 ++-- library/des.c | 77 +++++++++++++++-------------- library/ecp.c | 20 ++++---- library/ecp_curves.c | 60 ++++++++++++----------- library/havege.c | 2 +- library/md4.c | 30 +++++++++--- library/md5.c | 21 ++++---- library/oid.c | 53 ++++++++++---------- library/poly1305.c | 8 ++-- library/ripemd160.c | 33 ++++++++----- library/sha1.c | 32 +++++++------ library/sha256.c | 29 +++++------ library/sha512.c | 21 ++++---- library/x509.c | 11 ++++- library/x509_crt.c | 4 +- programs/ssl/ssl_server2.c | 34 +++++++------ programs/test/benchmark.c | 2 +- 23 files changed, 325 insertions(+), 252 deletions(-) diff --git a/include/mbedtls/asn1write.h b/include/mbedtls/asn1write.h index 360540a00..a19424369 100644 --- a/include/mbedtls/asn1write.h +++ b/include/mbedtls/asn1write.h @@ -33,11 +33,12 @@ #include "asn1.h" #define MBEDTLS_ASN1_CHK_ADD(g, f) \ - do { \ - if( ( ret = f ) < 0 ) \ + do \ + { \ + if( ( ret = (f) ) < 0 ) \ return( ret ); \ else \ - g += ret; \ + (g) += ret; \ } while( 0 ) #ifdef __cplusplus diff --git a/include/mbedtls/bignum.h b/include/mbedtls/bignum.h index a54c18e37..1c8607264 100644 --- a/include/mbedtls/bignum.h +++ b/include/mbedtls/bignum.h @@ -46,7 +46,12 @@ #define MBEDTLS_ERR_MPI_NOT_ACCEPTABLE -0x000E /**< The input arguments are not acceptable. */ #define MBEDTLS_ERR_MPI_ALLOC_FAILED -0x0010 /**< Memory allocation failed. */ -#define MBEDTLS_MPI_CHK(f) do { if( ( ret = f ) != 0 ) goto cleanup; } while( 0 ) +#define MBEDTLS_MPI_CHK(f) \ + do \ + { \ + if( ( ret = (f) ) != 0 ) \ + goto cleanup; \ + } while( 0 ) /* * Maximum size MPIs are allowed to grow to in number of limbs. diff --git a/include/mbedtls/padlock.h b/include/mbedtls/padlock.h index f05b72b07..721a5d493 100644 --- a/include/mbedtls/padlock.h +++ b/include/mbedtls/padlock.h @@ -59,7 +59,7 @@ #define MBEDTLS_PADLOCK_PHE 0x0C00 #define MBEDTLS_PADLOCK_PMM 0x3000 -#define MBEDTLS_PADLOCK_ALIGN16(x) (uint32_t *) (16 + ((int32_t) x & ~15)) +#define MBEDTLS_PADLOCK_ALIGN16(x) (uint32_t *) (16 + ((int32_t) (x) & ~15)) #ifdef __cplusplus extern "C" { diff --git a/include/mbedtls/x509_crt.h b/include/mbedtls/x509_crt.h index 3dd592248..670bd10d8 100644 --- a/include/mbedtls/x509_crt.h +++ b/include/mbedtls/x509_crt.h @@ -98,7 +98,7 @@ mbedtls_x509_crt; * Build flag from an algorithm/curve identifier (pk, md, ecp) * Since 0 is always XXX_NONE, ignore it. */ -#define MBEDTLS_X509_ID_FLAG( id ) ( 1 << ( id - 1 ) ) +#define MBEDTLS_X509_ID_FLAG( id ) ( 1 << ( (id) - 1 ) ) /** * Security profile for certificate verification. diff --git a/library/aes.c b/library/aes.c index 0543cd781..b2e346ec1 100644 --- a/library/aes.c +++ b/library/aes.c @@ -395,9 +395,9 @@ static uint32_t RCON[10]; /* * Tables generation code */ -#define ROTL8(x) ( ( x << 8 ) & 0xFFFFFFFF ) | ( x >> 24 ) -#define XTIME(x) ( ( x << 1 ) ^ ( ( x & 0x80 ) ? 0x1B : 0x00 ) ) -#define MUL(x,y) ( ( x && y ) ? pow[(log[x]+log[y]) % 255] : 0 ) +#define ROTL8(x) ( ( (x) << 8 ) & 0xFFFFFFFF ) | ( (x) >> 24 ) +#define XTIME(x) ( ( (x) << 1 ) ^ ( ( (x) & 0x80 ) ? 0x1B : 0x00 ) ) +#define MUL(x,y) ( ( (x) && (y) ) ? pow[(log[x]+log[y]) % 255] : 0 ) static int aes_init_done = 0; @@ -815,51 +815,53 @@ int mbedtls_aes_xts_setkey_dec( mbedtls_aes_xts_context *ctx, #endif /* !MBEDTLS_AES_SETKEY_DEC_ALT */ -#define AES_FROUND(X0,X1,X2,X3,Y0,Y1,Y2,Y3) \ -{ \ - X0 = *RK++ ^ AES_FT0( ( Y0 ) & 0xFF ) ^ \ - AES_FT1( ( Y1 >> 8 ) & 0xFF ) ^ \ - AES_FT2( ( Y2 >> 16 ) & 0xFF ) ^ \ - AES_FT3( ( Y3 >> 24 ) & 0xFF ); \ - \ - X1 = *RK++ ^ AES_FT0( ( Y1 ) & 0xFF ) ^ \ - AES_FT1( ( Y2 >> 8 ) & 0xFF ) ^ \ - AES_FT2( ( Y3 >> 16 ) & 0xFF ) ^ \ - AES_FT3( ( Y0 >> 24 ) & 0xFF ); \ - \ - X2 = *RK++ ^ AES_FT0( ( Y2 ) & 0xFF ) ^ \ - AES_FT1( ( Y3 >> 8 ) & 0xFF ) ^ \ - AES_FT2( ( Y0 >> 16 ) & 0xFF ) ^ \ - AES_FT3( ( Y1 >> 24 ) & 0xFF ); \ - \ - X3 = *RK++ ^ AES_FT0( ( Y3 ) & 0xFF ) ^ \ - AES_FT1( ( Y0 >> 8 ) & 0xFF ) ^ \ - AES_FT2( ( Y1 >> 16 ) & 0xFF ) ^ \ - AES_FT3( ( Y2 >> 24 ) & 0xFF ); \ -} +#define AES_FROUND(X0,X1,X2,X3,Y0,Y1,Y2,Y3) \ + do \ + { \ + (X0) = *RK++ ^ AES_FT0( ( (Y0) ) & 0xFF ) ^ \ + AES_FT1( ( (Y1) >> 8 ) & 0xFF ) ^ \ + AES_FT2( ( (Y2) >> 16 ) & 0xFF ) ^ \ + AES_FT3( ( (Y3) >> 24 ) & 0xFF ); \ + \ + (X1) = *RK++ ^ AES_FT0( ( (Y1) ) & 0xFF ) ^ \ + AES_FT1( ( (Y2) >> 8 ) & 0xFF ) ^ \ + AES_FT2( ( (Y3) >> 16 ) & 0xFF ) ^ \ + AES_FT3( ( (Y0) >> 24 ) & 0xFF ); \ + \ + (X2) = *RK++ ^ AES_FT0( ( (Y2) ) & 0xFF ) ^ \ + AES_FT1( ( (Y3) >> 8 ) & 0xFF ) ^ \ + AES_FT2( ( (Y0) >> 16 ) & 0xFF ) ^ \ + AES_FT3( ( (Y1) >> 24 ) & 0xFF ); \ + \ + (X3) = *RK++ ^ AES_FT0( ( (Y3) ) & 0xFF ) ^ \ + AES_FT1( ( (Y0) >> 8 ) & 0xFF ) ^ \ + AES_FT2( ( (Y1) >> 16 ) & 0xFF ) ^ \ + AES_FT3( ( (Y2) >> 24 ) & 0xFF ); \ + } while( 0 ) -#define AES_RROUND(X0,X1,X2,X3,Y0,Y1,Y2,Y3) \ -{ \ - X0 = *RK++ ^ AES_RT0( ( Y0 ) & 0xFF ) ^ \ - AES_RT1( ( Y3 >> 8 ) & 0xFF ) ^ \ - AES_RT2( ( Y2 >> 16 ) & 0xFF ) ^ \ - AES_RT3( ( Y1 >> 24 ) & 0xFF ); \ - \ - X1 = *RK++ ^ AES_RT0( ( Y1 ) & 0xFF ) ^ \ - AES_RT1( ( Y0 >> 8 ) & 0xFF ) ^ \ - AES_RT2( ( Y3 >> 16 ) & 0xFF ) ^ \ - AES_RT3( ( Y2 >> 24 ) & 0xFF ); \ - \ - X2 = *RK++ ^ AES_RT0( ( Y2 ) & 0xFF ) ^ \ - AES_RT1( ( Y1 >> 8 ) & 0xFF ) ^ \ - AES_RT2( ( Y0 >> 16 ) & 0xFF ) ^ \ - AES_RT3( ( Y3 >> 24 ) & 0xFF ); \ - \ - X3 = *RK++ ^ AES_RT0( ( Y3 ) & 0xFF ) ^ \ - AES_RT1( ( Y2 >> 8 ) & 0xFF ) ^ \ - AES_RT2( ( Y1 >> 16 ) & 0xFF ) ^ \ - AES_RT3( ( Y0 >> 24 ) & 0xFF ); \ -} +#define AES_RROUND(X0,X1,X2,X3,Y0,Y1,Y2,Y3) \ + do \ + { \ + (X0) = *RK++ ^ AES_RT0( ( (Y0) ) & 0xFF ) ^ \ + AES_RT1( ( (Y3) >> 8 ) & 0xFF ) ^ \ + AES_RT2( ( (Y2) >> 16 ) & 0xFF ) ^ \ + AES_RT3( ( (Y1) >> 24 ) & 0xFF ); \ + \ + (X1) = *RK++ ^ AES_RT0( ( (Y1) ) & 0xFF ) ^ \ + AES_RT1( ( (Y0) >> 8 ) & 0xFF ) ^ \ + AES_RT2( ( (Y3) >> 16 ) & 0xFF ) ^ \ + AES_RT3( ( (Y2) >> 24 ) & 0xFF ); \ + \ + (X2) = *RK++ ^ AES_RT0( ( (Y2) ) & 0xFF ) ^ \ + AES_RT1( ( (Y1) >> 8 ) & 0xFF ) ^ \ + AES_RT2( ( (Y0) >> 16 ) & 0xFF ) ^ \ + AES_RT3( ( (Y3) >> 24 ) & 0xFF ); \ + \ + (X3) = *RK++ ^ AES_RT0( ( (Y3) ) & 0xFF ) ^ \ + AES_RT1( ( (Y2) >> 8 ) & 0xFF ) ^ \ + AES_RT2( ( (Y1) >> 16 ) & 0xFF ) ^ \ + AES_RT3( ( (Y0) >> 24 ) & 0xFF ); \ + } while( 0 ) /* * AES-ECB block encryption diff --git a/library/ccm.c b/library/ccm.c index 01e58b043..c6211ee77 100644 --- a/library/ccm.c +++ b/library/ccm.c @@ -134,11 +134,17 @@ void mbedtls_ccm_free( mbedtls_ccm_context *ctx ) * This avoids allocating one more 16 bytes buffer while allowing src == dst. */ #define CTR_CRYPT( dst, src, len ) \ - if( ( ret = mbedtls_cipher_update( &ctx->cipher_ctx, ctr, 16, b, &olen ) ) != 0 ) \ - return( ret ); \ - \ - for( i = 0; i < len; i++ ) \ - dst[i] = src[i] ^ b[i]; + do \ + { \ + if( ( ret = mbedtls_cipher_update( &ctx->cipher_ctx, ctr, \ + 16, b, &olen ) ) != 0 ) \ + { \ + return( ret ); \ + } \ + \ + for( i = 0; i < (len); i++ ) \ + (dst)[i] = (src)[i] ^ b[i]; \ + } while( 0 ) /* * Authenticated encryption or decryption diff --git a/library/chacha20.c b/library/chacha20.c index 0757163e2..8a3610f0e 100644 --- a/library/chacha20.c +++ b/library/chacha20.c @@ -60,14 +60,14 @@ MBEDTLS_INTERNAL_VALIDATE( cond ) #define BYTES_TO_U32_LE( data, offset ) \ - ( (uint32_t) data[offset] \ - | (uint32_t) ( (uint32_t) data[( offset ) + 1] << 8 ) \ - | (uint32_t) ( (uint32_t) data[( offset ) + 2] << 16 ) \ - | (uint32_t) ( (uint32_t) data[( offset ) + 3] << 24 ) \ + ( (uint32_t) (data)[offset] \ + | (uint32_t) ( (uint32_t) (data)[( offset ) + 1] << 8 ) \ + | (uint32_t) ( (uint32_t) (data)[( offset ) + 2] << 16 ) \ + | (uint32_t) ( (uint32_t) (data)[( offset ) + 3] << 24 ) \ ) #define ROTL32( value, amount ) \ - ( (uint32_t) ( value << amount ) | ( value >> ( 32 - amount ) ) ) + ( (uint32_t) ( (value) << (amount) ) | ( (value) >> ( 32 - (amount) ) ) ) #define CHACHA20_CTR_INDEX ( 12U ) diff --git a/library/des.c b/library/des.c index ca9e071f3..8a33d82e5 100644 --- a/library/des.c +++ b/library/des.c @@ -257,50 +257,57 @@ static const uint32_t RHs[16] = /* * Initial Permutation macro */ -#define DES_IP(X,Y) \ -{ \ - T = ((X >> 4) ^ Y) & 0x0F0F0F0F; Y ^= T; X ^= (T << 4); \ - T = ((X >> 16) ^ Y) & 0x0000FFFF; Y ^= T; X ^= (T << 16); \ - T = ((Y >> 2) ^ X) & 0x33333333; X ^= T; Y ^= (T << 2); \ - T = ((Y >> 8) ^ X) & 0x00FF00FF; X ^= T; Y ^= (T << 8); \ - Y = ((Y << 1) | (Y >> 31)) & 0xFFFFFFFF; \ - T = (X ^ Y) & 0xAAAAAAAA; Y ^= T; X ^= T; \ - X = ((X << 1) | (X >> 31)) & 0xFFFFFFFF; \ -} +#define DES_IP(X,Y) \ + do \ + { \ + T = (((X) >> 4) ^ (Y)) & 0x0F0F0F0F; (Y) ^= T; (X) ^= (T << 4); \ + T = (((X) >> 16) ^ (Y)) & 0x0000FFFF; (Y) ^= T; (X) ^= (T << 16); \ + T = (((Y) >> 2) ^ (X)) & 0x33333333; (X) ^= T; (Y) ^= (T << 2); \ + T = (((Y) >> 8) ^ (X)) & 0x00FF00FF; (X) ^= T; (Y) ^= (T << 8); \ + (Y) = (((Y) << 1) | ((Y) >> 31)) & 0xFFFFFFFF; \ + T = ((X) ^ (Y)) & 0xAAAAAAAA; (Y) ^= T; (X) ^= T; \ + (X) = (((X) << 1) | ((X) >> 31)) & 0xFFFFFFFF; \ + } while( 0 ) /* * Final Permutation macro */ -#define DES_FP(X,Y) \ -{ \ - X = ((X << 31) | (X >> 1)) & 0xFFFFFFFF; \ - T = (X ^ Y) & 0xAAAAAAAA; X ^= T; Y ^= T; \ - Y = ((Y << 31) | (Y >> 1)) & 0xFFFFFFFF; \ - T = ((Y >> 8) ^ X) & 0x00FF00FF; X ^= T; Y ^= (T << 8); \ - T = ((Y >> 2) ^ X) & 0x33333333; X ^= T; Y ^= (T << 2); \ - T = ((X >> 16) ^ Y) & 0x0000FFFF; Y ^= T; X ^= (T << 16); \ - T = ((X >> 4) ^ Y) & 0x0F0F0F0F; Y ^= T; X ^= (T << 4); \ -} +#define DES_FP(X,Y) \ + do \ + { \ + (X) = (((X) << 31) | ((X) >> 1)) & 0xFFFFFFFF; \ + T = ((X) ^ (Y)) & 0xAAAAAAAA; (X) ^= T; (Y) ^= T; \ + (Y) = (((Y) << 31) | ((Y) >> 1)) & 0xFFFFFFFF; \ + T = (((Y) >> 8) ^ (X)) & 0x00FF00FF; (X) ^= T; (Y) ^= (T << 8); \ + T = (((Y) >> 2) ^ (X)) & 0x33333333; (X) ^= T; (Y) ^= (T << 2); \ + T = (((X) >> 16) ^ (Y)) & 0x0000FFFF; (Y) ^= T; (X) ^= (T << 16); \ + T = (((X) >> 4) ^ (Y)) & 0x0F0F0F0F; (Y) ^= T; (X) ^= (T << 4); \ + } while( 0 ) /* * DES round macro */ -#define DES_ROUND(X,Y) \ -{ \ - T = *SK++ ^ X; \ - Y ^= SB8[ (T ) & 0x3F ] ^ \ - SB6[ (T >> 8) & 0x3F ] ^ \ - SB4[ (T >> 16) & 0x3F ] ^ \ - SB2[ (T >> 24) & 0x3F ]; \ - \ - T = *SK++ ^ ((X << 28) | (X >> 4)); \ - Y ^= SB7[ (T ) & 0x3F ] ^ \ - SB5[ (T >> 8) & 0x3F ] ^ \ - SB3[ (T >> 16) & 0x3F ] ^ \ - SB1[ (T >> 24) & 0x3F ]; \ -} +#define DES_ROUND(X,Y) \ + do \ + { \ + T = *SK++ ^ (X); \ + (Y) ^= SB8[ (T ) & 0x3F ] ^ \ + SB6[ (T >> 8) & 0x3F ] ^ \ + SB4[ (T >> 16) & 0x3F ] ^ \ + SB2[ (T >> 24) & 0x3F ]; \ + \ + T = *SK++ ^ (((X) << 28) | ((X) >> 4)); \ + (Y) ^= SB7[ (T ) & 0x3F ] ^ \ + SB5[ (T >> 8) & 0x3F ] ^ \ + SB3[ (T >> 16) & 0x3F ] ^ \ + SB1[ (T >> 24) & 0x3F ]; \ + } while( 0 ) -#define SWAP(a,b) { uint32_t t = a; a = b; b = t; t = 0; } +#define SWAP(a,b) \ + do \ + { \ + uint32_t t = (a); (a) = (b); (b) = t; t = 0; \ + } while( 0 ) void mbedtls_des_init( mbedtls_des_context *ctx ) { diff --git a/library/ecp.c b/library/ecp.c index ecea5910e..db36191b9 100644 --- a/library/ecp.c +++ b/library/ecp.c @@ -1046,25 +1046,29 @@ cleanup: #define INC_MUL_COUNT #endif -#define MOD_MUL( N ) do { MBEDTLS_MPI_CHK( ecp_modp( &N, grp ) ); INC_MUL_COUNT } \ - while( 0 ) +#define MOD_MUL( N ) \ + do \ + { \ + MBEDTLS_MPI_CHK( ecp_modp( &(N), grp ) ); \ + INC_MUL_COUNT \ + } while( 0 ) /* * Reduce a mbedtls_mpi mod p in-place, to use after mbedtls_mpi_sub_mpi * N->s < 0 is a very fast test, which fails only if N is 0 */ -#define MOD_SUB( N ) \ - while( N.s < 0 && mbedtls_mpi_cmp_int( &N, 0 ) != 0 ) \ - MBEDTLS_MPI_CHK( mbedtls_mpi_add_mpi( &N, &N, &grp->P ) ) +#define MOD_SUB( N ) \ + while( (N).s < 0 && mbedtls_mpi_cmp_int( &(N), 0 ) != 0 ) \ + MBEDTLS_MPI_CHK( mbedtls_mpi_add_mpi( &(N), &(N), &grp->P ) ) /* * Reduce a mbedtls_mpi mod p in-place, to use after mbedtls_mpi_add_mpi and mbedtls_mpi_mul_int. * We known P, N and the result are positive, so sub_abs is correct, and * a bit faster. */ -#define MOD_ADD( N ) \ - while( mbedtls_mpi_cmp_mpi( &N, &grp->P ) >= 0 ) \ - MBEDTLS_MPI_CHK( mbedtls_mpi_sub_abs( &N, &N, &grp->P ) ) +#define MOD_ADD( N ) \ + while( mbedtls_mpi_cmp_mpi( &(N), &grp->P ) >= 0 ) \ + MBEDTLS_MPI_CHK( mbedtls_mpi_sub_abs( &(N), &(N), &grp->P ) ) #if defined(ECP_SHORTWEIERSTRASS) /* diff --git a/library/ecp_curves.c b/library/ecp_curves.c index 731621dc3..282481d05 100644 --- a/library/ecp_curves.c +++ b/library/ecp_curves.c @@ -51,11 +51,11 @@ */ #if defined(MBEDTLS_HAVE_INT32) -#define BYTES_TO_T_UINT_4( a, b, c, d ) \ - ( (mbedtls_mpi_uint) a << 0 ) | \ - ( (mbedtls_mpi_uint) b << 8 ) | \ - ( (mbedtls_mpi_uint) c << 16 ) | \ - ( (mbedtls_mpi_uint) d << 24 ) +#define BYTES_TO_T_UINT_4( a, b, c, d ) \ + ( (mbedtls_mpi_uint) (a) << 0 ) | \ + ( (mbedtls_mpi_uint) (b) << 8 ) | \ + ( (mbedtls_mpi_uint) (c) << 16 ) | \ + ( (mbedtls_mpi_uint) (d) << 24 ) #define BYTES_TO_T_UINT_2( a, b ) \ BYTES_TO_T_UINT_4( a, b, 0, 0 ) @@ -67,14 +67,14 @@ #else /* 64-bits */ #define BYTES_TO_T_UINT_8( a, b, c, d, e, f, g, h ) \ - ( (mbedtls_mpi_uint) a << 0 ) | \ - ( (mbedtls_mpi_uint) b << 8 ) | \ - ( (mbedtls_mpi_uint) c << 16 ) | \ - ( (mbedtls_mpi_uint) d << 24 ) | \ - ( (mbedtls_mpi_uint) e << 32 ) | \ - ( (mbedtls_mpi_uint) f << 40 ) | \ - ( (mbedtls_mpi_uint) g << 48 ) | \ - ( (mbedtls_mpi_uint) h << 56 ) + ( (mbedtls_mpi_uint) (a) << 0 ) | \ + ( (mbedtls_mpi_uint) (b) << 8 ) | \ + ( (mbedtls_mpi_uint) (c) << 16 ) | \ + ( (mbedtls_mpi_uint) (d) << 24 ) | \ + ( (mbedtls_mpi_uint) (e) << 32 ) | \ + ( (mbedtls_mpi_uint) (f) << 40 ) | \ + ( (mbedtls_mpi_uint) (g) << 48 ) | \ + ( (mbedtls_mpi_uint) (h) << 56 ) #define BYTES_TO_T_UINT_4( a, b, c, d ) \ BYTES_TO_T_UINT_8( a, b, c, d, 0, 0, 0, 0 ) @@ -890,7 +890,7 @@ static inline void carry64( mbedtls_mpi_uint *dst, mbedtls_mpi_uint *carry ) } #define WIDTH 8 / sizeof( mbedtls_mpi_uint ) -#define A( i ) N->p + i * WIDTH +#define A( i ) N->p + (i) * WIDTH #define ADD( i ) add64( p, A( i ), &c ) #define NEXT p += WIDTH; carry64( p, &c ) #define LAST p += WIDTH; *p = c; while( ++p < end ) *p = 0 @@ -955,7 +955,8 @@ cleanup: #else /* 64-bit */ #define MAX32 N->n * 2 -#define A( j ) j % 2 ? (uint32_t)( N->p[j/2] >> 32 ) : (uint32_t)( N->p[j/2] ) +#define A( j ) (j) % 2 ? (uint32_t)( N->p[(j)/2] >> 32 ) : \ + (uint32_t)( N->p[(j)/2] ) #define STORE32 \ if( i % 2 ) { \ N->p[i/2] &= 0x00000000FFFFFFFF; \ @@ -989,20 +990,21 @@ static inline void sub32( uint32_t *dst, uint32_t src, signed char *carry ) * Helpers for the main 'loop' * (see fix_negative for the motivation of C) */ -#define INIT( b ) \ - int ret; \ - signed char c = 0, cc; \ - uint32_t cur; \ - size_t i = 0, bits = b; \ - mbedtls_mpi C; \ - mbedtls_mpi_uint Cp[ b / 8 / sizeof( mbedtls_mpi_uint) + 1 ]; \ - \ - C.s = 1; \ - C.n = b / 8 / sizeof( mbedtls_mpi_uint) + 1; \ - C.p = Cp; \ - memset( Cp, 0, C.n * sizeof( mbedtls_mpi_uint ) ); \ - \ - MBEDTLS_MPI_CHK( mbedtls_mpi_grow( N, b * 2 / 8 / sizeof( mbedtls_mpi_uint ) ) ); \ +#define INIT( b ) \ + int ret; \ + signed char c = 0, cc; \ + uint32_t cur; \ + size_t i = 0, bits = (b); \ + mbedtls_mpi C; \ + mbedtls_mpi_uint Cp[ (b) / 8 / sizeof( mbedtls_mpi_uint) + 1 ]; \ + \ + C.s = 1; \ + C.n = (b) / 8 / sizeof( mbedtls_mpi_uint) + 1; \ + C.p = Cp; \ + memset( Cp, 0, C.n * sizeof( mbedtls_mpi_uint ) ); \ + \ + MBEDTLS_MPI_CHK( mbedtls_mpi_grow( N, (b) * 2 / 8 / \ + sizeof( mbedtls_mpi_uint ) ) ); \ LOAD32; #define NEXT \ diff --git a/library/havege.c b/library/havege.c index 4dcac0287..54f897c6e 100644 --- a/library/havege.c +++ b/library/havege.c @@ -54,7 +54,7 @@ * ------------------------------------------------------------------------ */ -#define SWAP(X,Y) { int *T = X; X = Y; Y = T; } +#define SWAP(X,Y) { int *T = (X); (X) = (Y); (Y) = T; } #define TST1_ENTER if( PTEST & 1 ) { PTEST ^= 3; PTEST >>= 1; #define TST2_ENTER if( PTEST & 1 ) { PTEST ^= 3; PTEST >>= 1; diff --git a/library/md4.c b/library/md4.c index 3f8ddff31..156e71534 100644 --- a/library/md4.c +++ b/library/md4.c @@ -137,15 +137,21 @@ int mbedtls_internal_md4_process( mbedtls_md4_context *ctx, GET_UINT32_LE( X[14], data, 56 ); GET_UINT32_LE( X[15], data, 60 ); -#define S(x,n) ((x << n) | ((x & 0xFFFFFFFF) >> (32 - n))) +#define S(x,n) (((x) << (n)) | (((x) & 0xFFFFFFFF) >> (32 - (n)))) A = ctx->state[0]; B = ctx->state[1]; C = ctx->state[2]; D = ctx->state[3]; -#define F(x, y, z) ((x & y) | ((~x) & z)) -#define P(a,b,c,d,x,s) { a += F(b,c,d) + x; a = S(a,s); } +#define F(x, y, z) (((x) & (y)) | ((~(x)) & (z))) +#define P(a,b,c,d,x,s) \ + do \ + { \ + (a) += F(b,c,d) + (x); \ + (a) = S(a,s); \ + } while( 0 ) + P( A, B, C, D, X[ 0], 3 ); P( D, A, B, C, X[ 1], 7 ); @@ -167,8 +173,13 @@ int mbedtls_internal_md4_process( mbedtls_md4_context *ctx, #undef P #undef F -#define F(x,y,z) ((x & y) | (x & z) | (y & z)) -#define P(a,b,c,d,x,s) { a += F(b,c,d) + x + 0x5A827999; a = S(a,s); } +#define F(x,y,z) (((x) & (y)) | ((x) & (z)) | ((y) & (z))) +#define P(a,b,c,d,x,s) \ + do \ + { \ + (a) += F(b,c,d) + (x) + 0x5A827999; \ + (a) = S(a,s); \ + } while( 0 ) P( A, B, C, D, X[ 0], 3 ); P( D, A, B, C, X[ 4], 5 ); @@ -190,8 +201,13 @@ int mbedtls_internal_md4_process( mbedtls_md4_context *ctx, #undef P #undef F -#define F(x,y,z) (x ^ y ^ z) -#define P(a,b,c,d,x,s) { a += F(b,c,d) + x + 0x6ED9EBA1; a = S(a,s); } +#define F(x,y,z) ((x) ^ (y) ^ (z)) +#define P(a,b,c,d,x,s) \ + do \ + { \ + (a) += F(b,c,d) + (x) + 0x6ED9EBA1; \ + (a) = S(a,s); \ + } while( 0 ) P( A, B, C, D, X[ 0], 3 ); P( D, A, B, C, X[ 8], 9 ); diff --git a/library/md5.c b/library/md5.c index 2a740cda8..bdbb760ed 100644 --- a/library/md5.c +++ b/library/md5.c @@ -136,19 +136,22 @@ int mbedtls_internal_md5_process( mbedtls_md5_context *ctx, GET_UINT32_LE( X[14], data, 56 ); GET_UINT32_LE( X[15], data, 60 ); -#define S(x,n) ((x << n) | ((x & 0xFFFFFFFF) >> (32 - n))) +#define S(x,n) \ + ( ( (x) << (n) ) | ( ( (x) & 0xFFFFFFFF) >> ( 32 - (n) ) ) ) -#define P(a,b,c,d,k,s,t) \ -{ \ - a += F(b,c,d) + X[k] + t; a = S(a,s) + b; \ -} +#define P(a,b,c,d,k,s,t) \ + do \ + { \ + (a) += F((b),(c),(d)) + X[k] + (t); \ + (a) = S((a),(s)) + (b); \ + } while( 0 ) A = ctx->state[0]; B = ctx->state[1]; C = ctx->state[2]; D = ctx->state[3]; -#define F(x,y,z) (z ^ (x & (y ^ z))) +#define F(x,y,z) ((z) ^ ((x) & ((y) ^ (z)))) P( A, B, C, D, 0, 7, 0xD76AA478 ); P( D, A, B, C, 1, 12, 0xE8C7B756 ); @@ -169,7 +172,7 @@ int mbedtls_internal_md5_process( mbedtls_md5_context *ctx, #undef F -#define F(x,y,z) (y ^ (z & (x ^ y))) +#define F(x,y,z) ((y) ^ ((z) & ((x) ^ (y)))) P( A, B, C, D, 1, 5, 0xF61E2562 ); P( D, A, B, C, 6, 9, 0xC040B340 ); @@ -190,7 +193,7 @@ int mbedtls_internal_md5_process( mbedtls_md5_context *ctx, #undef F -#define F(x,y,z) (x ^ y ^ z) +#define F(x,y,z) ((x) ^ (y) ^ (z)) P( A, B, C, D, 5, 4, 0xFFFA3942 ); P( D, A, B, C, 8, 11, 0x8771F681 ); @@ -211,7 +214,7 @@ int mbedtls_internal_md5_process( mbedtls_md5_context *ctx, #undef F -#define F(x,y,z) (y ^ (x | ~z)) +#define F(x,y,z) ((y) ^ ((x) | ~(z))) P( A, B, C, D, 0, 6, 0xF4292244 ); P( D, A, B, C, 7, 10, 0x432AFF97 ); diff --git a/library/oid.c b/library/oid.c index edea950f8..33f437cbe 100644 --- a/library/oid.c +++ b/library/oid.c @@ -54,22 +54,24 @@ * Macro to generate an internal function for oid_XXX_from_asn1() (used by * the other functions) */ -#define FN_OID_TYPED_FROM_ASN1( TYPE_T, NAME, LIST ) \ -static const TYPE_T * oid_ ## NAME ## _from_asn1( const mbedtls_asn1_buf *oid ) \ -{ \ - const TYPE_T *p = LIST; \ - const mbedtls_oid_descriptor_t *cur = (const mbedtls_oid_descriptor_t *) p; \ - if( p == NULL || oid == NULL ) return( NULL ); \ - while( cur->asn1 != NULL ) { \ - if( cur->asn1_len == oid->len && \ - memcmp( cur->asn1, oid->p, oid->len ) == 0 ) { \ - return( p ); \ - } \ - p++; \ - cur = (const mbedtls_oid_descriptor_t *) p; \ - } \ - return( NULL ); \ -} +#define FN_OID_TYPED_FROM_ASN1( TYPE_T, NAME, LIST ) \ + static const TYPE_T * oid_ ## NAME ## _from_asn1( \ + const mbedtls_asn1_buf *oid ) \ + { \ + const TYPE_T *p = (LIST); \ + const mbedtls_oid_descriptor_t *cur = \ + (const mbedtls_oid_descriptor_t *) p; \ + if( p == NULL || oid == NULL ) return( NULL ); \ + while( cur->asn1 != NULL ) { \ + if( cur->asn1_len == oid->len && \ + memcmp( cur->asn1, oid->p, oid->len ) == 0 ) { \ + return( p ); \ + } \ + p++; \ + cur = (const mbedtls_oid_descriptor_t *) p; \ + } \ + return( NULL ); \ + } /* * Macro to generate a function for retrieving a single attribute from the @@ -103,12 +105,13 @@ int FN_NAME( const mbedtls_asn1_buf *oid, ATTR1_TYPE * ATTR1 ) */ #define FN_OID_GET_ATTR2(FN_NAME, TYPE_T, TYPE_NAME, ATTR1_TYPE, ATTR1, \ ATTR2_TYPE, ATTR2) \ -int FN_NAME( const mbedtls_asn1_buf *oid, ATTR1_TYPE * ATTR1, ATTR2_TYPE * ATTR2 ) \ +int FN_NAME( const mbedtls_asn1_buf *oid, ATTR1_TYPE * ATTR1, \ + ATTR2_TYPE * ATTR2 ) \ { \ const TYPE_T *data = oid_ ## TYPE_NAME ## _from_asn1( oid ); \ - if( data == NULL ) return( MBEDTLS_ERR_OID_NOT_FOUND ); \ - *ATTR1 = data->ATTR1; \ - *ATTR2 = data->ATTR2; \ + if( data == NULL ) return( MBEDTLS_ERR_OID_NOT_FOUND ); \ + *(ATTR1) = data->ATTR1; \ + *(ATTR2) = data->ATTR2; \ return( 0 ); \ } @@ -119,16 +122,16 @@ int FN_NAME( const mbedtls_asn1_buf *oid, ATTR1_TYPE * ATTR1, ATTR2_TYPE * ATTR2 #define FN_OID_GET_OID_BY_ATTR1(FN_NAME, TYPE_T, LIST, ATTR1_TYPE, ATTR1) \ int FN_NAME( ATTR1_TYPE ATTR1, const char **oid, size_t *olen ) \ { \ - const TYPE_T *cur = LIST; \ + const TYPE_T *cur = (LIST); \ while( cur->descriptor.asn1 != NULL ) { \ - if( cur->ATTR1 == ATTR1 ) { \ + if( cur->ATTR1 == (ATTR1) ) { \ *oid = cur->descriptor.asn1; \ *olen = cur->descriptor.asn1_len; \ return( 0 ); \ } \ cur++; \ } \ - return( MBEDTLS_ERR_OID_NOT_FOUND ); \ + return( MBEDTLS_ERR_OID_NOT_FOUND ); \ } /* @@ -140,9 +143,9 @@ int FN_NAME( ATTR1_TYPE ATTR1, const char **oid, size_t *olen ) \ int FN_NAME( ATTR1_TYPE ATTR1, ATTR2_TYPE ATTR2, const char **oid , \ size_t *olen ) \ { \ - const TYPE_T *cur = LIST; \ + const TYPE_T *cur = (LIST); \ while( cur->descriptor.asn1 != NULL ) { \ - if( cur->ATTR1 == ATTR1 && cur->ATTR2 == ATTR2 ) { \ + if( cur->ATTR1 == (ATTR1) && cur->ATTR2 == (ATTR2) ) { \ *oid = cur->descriptor.asn1; \ *olen = cur->descriptor.asn1_len; \ return( 0 ); \ diff --git a/library/poly1305.c b/library/poly1305.c index b27411918..2b56c5f7e 100644 --- a/library/poly1305.c +++ b/library/poly1305.c @@ -58,10 +58,10 @@ #define POLY1305_BLOCK_SIZE_BYTES ( 16U ) #define BYTES_TO_U32_LE( data, offset ) \ - ( (uint32_t) data[offset] \ - | (uint32_t) ( (uint32_t) data[( offset ) + 1] << 8 ) \ - | (uint32_t) ( (uint32_t) data[( offset ) + 2] << 16 ) \ - | (uint32_t) ( (uint32_t) data[( offset ) + 3] << 24 ) \ + ( (uint32_t) (data)[offset] \ + | (uint32_t) ( (uint32_t) (data)[( offset ) + 1] << 8 ) \ + | (uint32_t) ( (uint32_t) (data)[( offset ) + 2] << 16 ) \ + | (uint32_t) ( (uint32_t) (data)[( offset ) + 3] << 24 ) \ ) /* diff --git a/library/ripemd160.c b/library/ripemd160.c index bd25ada62..0791ae4cc 100644 --- a/library/ripemd160.c +++ b/library/ripemd160.c @@ -147,22 +147,29 @@ int mbedtls_internal_ripemd160_process( mbedtls_ripemd160_context *ctx, D = Dp = ctx->state[3]; E = Ep = ctx->state[4]; -#define F1( x, y, z ) ( x ^ y ^ z ) -#define F2( x, y, z ) ( ( x & y ) | ( ~x & z ) ) -#define F3( x, y, z ) ( ( x | ~y ) ^ z ) -#define F4( x, y, z ) ( ( x & z ) | ( y & ~z ) ) -#define F5( x, y, z ) ( x ^ ( y | ~z ) ) +#define F1( x, y, z ) ( (x) ^ (y) ^ (z) ) +#define F2( x, y, z ) ( ( (x) & (y) ) | ( ~(x) & (z) ) ) +#define F3( x, y, z ) ( ( (x) | ~(y) ) ^ (z) ) +#define F4( x, y, z ) ( ( (x) & (z) ) | ( (y) & ~(z) ) ) +#define F5( x, y, z ) ( (x) ^ ( (y) | ~(z) ) ) -#define S( x, n ) ( ( x << n ) | ( x >> (32 - n) ) ) +#define S( x, n ) ( ( (x) << (n) ) | ( (x) >> (32 - (n)) ) ) -#define P( a, b, c, d, e, r, s, f, k ) \ - a += f( b, c, d ) + X[r] + k; \ - a = S( a, s ) + e; \ - c = S( c, 10 ); +#define P( a, b, c, d, e, r, s, f, k ) \ + do \ + { \ + (a) += f( (b), (c), (d) ) + X[r] + (k); \ + (a) = S( (a), (s) ) + (e); \ + (c) = S( (c), 10 ); \ + } while( 0 ) -#define P2( a, b, c, d, e, r, s, rp, sp ) \ - P( a, b, c, d, e, r, s, F, K ); \ - P( a ## p, b ## p, c ## p, d ## p, e ## p, rp, sp, Fp, Kp ); +#define P2( a, b, c, d, e, r, s, rp, sp ) \ + do \ + { \ + P( (a), (b), (c), (d), (e), (r), (s), F, K ); \ + P( a ## p, b ## p, c ## p, d ## p, e ## p, \ + (rp), (sp), Fp, Kp ); \ + } while( 0 ) #define F F1 #define K 0x00000000 diff --git a/library/sha1.c b/library/sha1.c index e8d4096fb..2f2946b55 100644 --- a/library/sha1.c +++ b/library/sha1.c @@ -152,19 +152,21 @@ int mbedtls_internal_sha1_process( mbedtls_sha1_context *ctx, GET_UINT32_BE( W[14], data, 56 ); GET_UINT32_BE( W[15], data, 60 ); -#define S(x,n) ((x << n) | ((x & 0xFFFFFFFF) >> (32 - n))) +#define S(x,n) (((x) << (n)) | (((x) & 0xFFFFFFFF) >> (32 - (n)))) -#define R(t) \ -( \ - temp = W[( t - 3 ) & 0x0F] ^ W[( t - 8 ) & 0x0F] ^ \ - W[( t - 14 ) & 0x0F] ^ W[ t & 0x0F], \ - ( W[t & 0x0F] = S(temp,1) ) \ -) +#define R(t) \ + ( \ + temp = W[( (t) - 3 ) & 0x0F] ^ W[( (t) - 8 ) & 0x0F] ^ \ + W[( (t) - 14 ) & 0x0F] ^ W[ (t) & 0x0F], \ + ( W[(t) & 0x0F] = S(temp,1) ) \ + ) -#define P(a,b,c,d,e,x) \ -{ \ - e += S(a,5) + F(b,c,d) + K + x; b = S(b,30); \ -} +#define P(a,b,c,d,e,x) \ + do \ + { \ + (e) += S(a,5) + F(b,c,d) + K + (x); \ + (b) = S(b,30); \ + } while( 0 ) A = ctx->state[0]; B = ctx->state[1]; @@ -172,7 +174,7 @@ int mbedtls_internal_sha1_process( mbedtls_sha1_context *ctx, D = ctx->state[3]; E = ctx->state[4]; -#define F(x,y,z) (z ^ (x & (y ^ z))) +#define F(x,y,z) ((z) ^ ((x) & ((y) ^ (z)))) #define K 0x5A827999 P( A, B, C, D, E, W[0] ); @@ -199,7 +201,7 @@ int mbedtls_internal_sha1_process( mbedtls_sha1_context *ctx, #undef K #undef F -#define F(x,y,z) (x ^ y ^ z) +#define F(x,y,z) ((x) ^ (y) ^ (z)) #define K 0x6ED9EBA1 P( A, B, C, D, E, R(20) ); @@ -226,7 +228,7 @@ int mbedtls_internal_sha1_process( mbedtls_sha1_context *ctx, #undef K #undef F -#define F(x,y,z) ((x & y) | (z & (x | y))) +#define F(x,y,z) (((x) & (y)) | ((z) & ((x) | (y)))) #define K 0x8F1BBCDC P( A, B, C, D, E, R(40) ); @@ -253,7 +255,7 @@ int mbedtls_internal_sha1_process( mbedtls_sha1_context *ctx, #undef K #undef F -#define F(x,y,z) (x ^ y ^ z) +#define F(x,y,z) ((x) ^ (y) ^ (z)) #define K 0xCA62C1D6 P( A, B, C, D, E, R(60) ); diff --git a/library/sha256.c b/library/sha256.c index 8a540adfb..09d3fe349 100644 --- a/library/sha256.c +++ b/library/sha256.c @@ -172,8 +172,8 @@ static const uint32_t K[] = 0x90BEFFFA, 0xA4506CEB, 0xBEF9A3F7, 0xC67178F2, }; -#define SHR(x,n) ((x & 0xFFFFFFFF) >> n) -#define ROTR(x,n) (SHR(x,n) | (x << (32 - n))) +#define SHR(x,n) (((x) & 0xFFFFFFFF) >> (n)) +#define ROTR(x,n) (SHR(x,n) | ((x) << (32 - (n)))) #define S0(x) (ROTR(x, 7) ^ ROTR(x,18) ^ SHR(x, 3)) #define S1(x) (ROTR(x,17) ^ ROTR(x,19) ^ SHR(x,10)) @@ -181,21 +181,22 @@ static const uint32_t K[] = #define S2(x) (ROTR(x, 2) ^ ROTR(x,13) ^ ROTR(x,22)) #define S3(x) (ROTR(x, 6) ^ ROTR(x,11) ^ ROTR(x,25)) -#define F0(x,y,z) ((x & y) | (z & (x | y))) -#define F1(x,y,z) (z ^ (x & (y ^ z))) +#define F0(x,y,z) (((x) & (y)) | ((z) & ((x) | (y)))) +#define F1(x,y,z) ((z) ^ ((x) & ((y) ^ (z)))) #define R(t) \ -( \ - W[t] = S1(W[t - 2]) + W[t - 7] + \ - S0(W[t - 15]) + W[t - 16] \ -) + ( \ + W[t] = S1(W[(t) - 2]) + W[(t) - 7] + \ + S0(W[(t) - 15]) + W[(t) - 16] \ + ) -#define P(a,b,c,d,e,f,g,h,x,K) \ -{ \ - temp1 = h + S3(e) + F1(e,f,g) + K + x; \ - temp2 = S2(a) + F0(a,b,c); \ - d += temp1; h = temp1 + temp2; \ -} +#define P(a,b,c,d,e,f,g,h,x,K) \ + do \ + { \ + temp1 = (h) + S3(e) + F1(e,f,g) + (K) + (x); \ + temp2 = S2(a) + F0(a,b,c); \ + (d) += temp1; (h) = temp1 + temp2; \ + } while( 0 ) int mbedtls_internal_sha256_process( mbedtls_sha256_context *ctx, const unsigned char data[64] ) diff --git a/library/sha512.c b/library/sha512.c index 941ecda76..cec955b4c 100644 --- a/library/sha512.c +++ b/library/sha512.c @@ -224,8 +224,8 @@ int mbedtls_internal_sha512_process( mbedtls_sha512_context *ctx, SHA512_VALIDATE_RET( ctx != NULL ); SHA512_VALIDATE_RET( (const unsigned char *)data != NULL ); -#define SHR(x,n) (x >> n) -#define ROTR(x,n) (SHR(x,n) | (x << (64 - n))) +#define SHR(x,n) ((x) >> (n)) +#define ROTR(x,n) (SHR(x,n) | ((x) << (64 - (n)))) #define S0(x) (ROTR(x, 1) ^ ROTR(x, 8) ^ SHR(x, 7)) #define S1(x) (ROTR(x,19) ^ ROTR(x,61) ^ SHR(x, 6)) @@ -233,15 +233,16 @@ int mbedtls_internal_sha512_process( mbedtls_sha512_context *ctx, #define S2(x) (ROTR(x,28) ^ ROTR(x,34) ^ ROTR(x,39)) #define S3(x) (ROTR(x,14) ^ ROTR(x,18) ^ ROTR(x,41)) -#define F0(x,y,z) ((x & y) | (z & (x | y))) -#define F1(x,y,z) (z ^ (x & (y ^ z))) +#define F0(x,y,z) (((x) & (y)) | ((z) & ((x) | (y)))) +#define F1(x,y,z) ((z) ^ ((x) & ((y) ^ (z)))) -#define P(a,b,c,d,e,f,g,h,x,K) \ -{ \ - temp1 = h + S3(e) + F1(e,f,g) + K + x; \ - temp2 = S2(a) + F0(a,b,c); \ - d += temp1; h = temp1 + temp2; \ -} +#define P(a,b,c,d,e,f,g,h,x,K) \ + do \ + { \ + temp1 = (h) + S3(e) + F1(e,f,g) + (K) + (x); \ + temp2 = S2(a) + F0(a,b,c); \ + (d) += temp1; (h) = temp1 + temp2; \ + } while( 0 ) for( i = 0; i < 16; i++ ) { diff --git a/library/x509.c b/library/x509.c index 7cc813ec6..aaf230105 100644 --- a/library/x509.c +++ b/library/x509.c @@ -67,8 +67,15 @@ #include #endif -#define CHECK(code) if( ( ret = code ) != 0 ){ return( ret ); } -#define CHECK_RANGE(min, max, val) if( val < min || val > max ){ return( ret ); } +#define CHECK(code) if( ( ret = ( code ) ) != 0 ){ return( ret ); } +#define CHECK_RANGE(min, max, val) \ + do \ + { \ + if( ( val ) < ( min ) || ( val ) > ( max ) ) \ + { \ + return( ret ); \ + } \ + } while( 0 ) /* * CertificateSerialNumber ::= INTEGER diff --git a/library/x509_crt.c b/library/x509_crt.c index 325bbc0b1..ebd118db0 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1439,7 +1439,7 @@ static int x509_info_subject_alt_name( char **buf, size_t *size, } #define CERT_TYPE(type,name) \ - if( ns_cert_type & type ) \ + if( ns_cert_type & (type) ) \ PRINT_ITEM( name ); static int x509_info_cert_type( char **buf, size_t *size, @@ -1466,7 +1466,7 @@ static int x509_info_cert_type( char **buf, size_t *size, } #define KEY_USAGE(code,name) \ - if( key_usage & code ) \ + if( key_usage & (code) ) \ PRINT_ITEM( name ); static int x509_info_key_usage( char **buf, size_t *size, diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 2a499adf9..ee482344a 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -586,11 +586,14 @@ static int get_auth_mode( const char *s ) * Used by sni_parse and psk_parse to handle coma-separated lists */ #define GET_ITEM( dst ) \ - dst = p; \ - while( *p != ',' ) \ - if( ++p > end ) \ - goto error; \ - *p++ = '\0'; + do \ + { \ + (dst) = p; \ + while( *p != ',' ) \ + if( ++p > end ) \ + goto error; \ + *p++ = '\0'; \ + } while( 0 ) #if defined(SNI_OPTION) typedef struct _sni_entry sni_entry; @@ -747,15 +750,18 @@ int sni_callback( void *p_info, mbedtls_ssl_context *ssl, #if defined(MBEDTLS_KEY_EXCHANGE__SOME__PSK_ENABLED) -#define HEX2NUM( c ) \ - if( c >= '0' && c <= '9' ) \ - c -= '0'; \ - else if( c >= 'a' && c <= 'f' ) \ - c -= 'a' - 10; \ - else if( c >= 'A' && c <= 'F' ) \ - c -= 'A' - 10; \ - else \ - return( -1 ); +#define HEX2NUM( c ) \ + do \ + { \ + if( (c) >= '0' && (c) <= '9' ) \ + (c) -= '0'; \ + else if( (c) >= 'a' && (c) <= 'f' ) \ + (c) -= 'a' - 10; \ + else if( (c) >= 'A' && (c) <= 'F' ) \ + (c) -= 'A' - 10; \ + else \ + return( -1 ); \ + } while( 0 ) /* * Convert a hex string to bytes. diff --git a/programs/test/benchmark.c b/programs/test/benchmark.c index 8d7ecf7c9..e31faafeb 100644 --- a/programs/test/benchmark.c +++ b/programs/test/benchmark.c @@ -163,7 +163,7 @@ do { \ #define MEMORY_MEASURE_PRINT( title_len ) \ mbedtls_memory_buffer_alloc_max_get( &max_used, &max_blocks ); \ - for( ii = 12 - title_len; ii != 0; ii-- ) mbedtls_printf( " " ); \ + for( ii = 12 - (title_len); ii != 0; ii-- ) mbedtls_printf( " " ); \ max_used -= prv_used; \ max_blocks -= prv_blocks; \ max_bytes = max_used + MEM_BLOCK_OVERHEAD * max_blocks; \ From ee60034a60d944d6abfa3a5f8a6767528040a2e1 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 15 Oct 2018 12:23:02 +0100 Subject: [PATCH 30/32] Adapt ChangeLog --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 978b8b5cd..9e46fdc06 100644 --- a/ChangeLog +++ b/ChangeLog @@ -32,6 +32,10 @@ Bugfix GCM and CCM were not affected. Fixed by Jack Lloyd. * Fix incorrect default port number in ssl_mail_client example's usage. Found and fixed by irwir. #2337 + * Add missing parentheses around parameters in the definition of the + public macro MBEDTLS_X509_ID_FLAG. This could lead to invalid evaluation + in case operators binding less strongly than subtraction were used + for the parameter. Changes * Return from various debugging routines immediately if the From 3ac21aca9b7afbbba2881844856b2e7421e76f0d Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 26 Oct 2018 09:13:26 +0100 Subject: [PATCH 31/32] Add further missing brackets around macro parameters --- library/aes.c | 2 +- library/md4.c | 4 ++-- library/md5.c | 2 +- library/sha1.c | 4 ++-- library/sha256.c | 4 ++-- library/sha512.c | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/library/aes.c b/library/aes.c index b2e346ec1..aff0a9939 100644 --- a/library/aes.c +++ b/library/aes.c @@ -397,7 +397,7 @@ static uint32_t RCON[10]; */ #define ROTL8(x) ( ( (x) << 8 ) & 0xFFFFFFFF ) | ( (x) >> 24 ) #define XTIME(x) ( ( (x) << 1 ) ^ ( ( (x) & 0x80 ) ? 0x1B : 0x00 ) ) -#define MUL(x,y) ( ( (x) && (y) ) ? pow[(log[x]+log[y]) % 255] : 0 ) +#define MUL(x,y) ( ( (x) && (y) ) ? pow[(log[(x)]+log[(y)]) % 255] : 0 ) static int aes_init_done = 0; diff --git a/library/md4.c b/library/md4.c index 156e71534..41c5d545f 100644 --- a/library/md4.c +++ b/library/md4.c @@ -148,8 +148,8 @@ int mbedtls_internal_md4_process( mbedtls_md4_context *ctx, #define P(a,b,c,d,x,s) \ do \ { \ - (a) += F(b,c,d) + (x); \ - (a) = S(a,s); \ + (a) += F((b),(c),(d)) + (x); \ + (a) = S((a),(s)); \ } while( 0 ) diff --git a/library/md5.c b/library/md5.c index bdbb760ed..a93da8a06 100644 --- a/library/md5.c +++ b/library/md5.c @@ -142,7 +142,7 @@ int mbedtls_internal_md5_process( mbedtls_md5_context *ctx, #define P(a,b,c,d,k,s,t) \ do \ { \ - (a) += F((b),(c),(d)) + X[k] + (t); \ + (a) += F((b),(c),(d)) + X[(k)] + (t); \ (a) = S((a),(s)) + (b); \ } while( 0 ) diff --git a/library/sha1.c b/library/sha1.c index 2f2946b55..355c83d2f 100644 --- a/library/sha1.c +++ b/library/sha1.c @@ -164,8 +164,8 @@ int mbedtls_internal_sha1_process( mbedtls_sha1_context *ctx, #define P(a,b,c,d,e,x) \ do \ { \ - (e) += S(a,5) + F(b,c,d) + K + (x); \ - (b) = S(b,30); \ + (e) += S((a),5) + F((b),(c),(d)) + K + (x); \ + (b) = S((b),30); \ } while( 0 ) A = ctx->state[0]; diff --git a/library/sha256.c b/library/sha256.c index 09d3fe349..2dc0e1a2c 100644 --- a/library/sha256.c +++ b/library/sha256.c @@ -193,8 +193,8 @@ static const uint32_t K[] = #define P(a,b,c,d,e,f,g,h,x,K) \ do \ { \ - temp1 = (h) + S3(e) + F1(e,f,g) + (K) + (x); \ - temp2 = S2(a) + F0(a,b,c); \ + temp1 = (h) + S3(e) + F1((e),(f),(g)) + (K) + (x); \ + temp2 = S2(a) + F0((a),(b),(c)); \ (d) += temp1; (h) = temp1 + temp2; \ } while( 0 ) diff --git a/library/sha512.c b/library/sha512.c index cec955b4c..efc4acb40 100644 --- a/library/sha512.c +++ b/library/sha512.c @@ -239,8 +239,8 @@ int mbedtls_internal_sha512_process( mbedtls_sha512_context *ctx, #define P(a,b,c,d,e,f,g,h,x,K) \ do \ { \ - temp1 = (h) + S3(e) + F1(e,f,g) + (K) + (x); \ - temp2 = S2(a) + F0(a,b,c); \ + temp1 = (h) + S3(e) + F1((e),(f),(g)) + (K) + (x); \ + temp2 = S2(a) + F0((a),(b),(c)); \ (d) += temp1; (h) = temp1 + temp2; \ } while( 0 ) From 9306f1c65d0edb8094d39d373d753bc757d2adcf Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 30 Oct 2018 09:29:25 +0000 Subject: [PATCH 32/32] Add more missing parentheses around macro parameters --- library/md4.c | 20 ++++++++++---------- library/sha512.c | 10 +++++----- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/library/md4.c b/library/md4.c index 41c5d545f..828fd4299 100644 --- a/library/md4.c +++ b/library/md4.c @@ -145,9 +145,9 @@ int mbedtls_internal_md4_process( mbedtls_md4_context *ctx, D = ctx->state[3]; #define F(x, y, z) (((x) & (y)) | ((~(x)) & (z))) -#define P(a,b,c,d,x,s) \ - do \ - { \ +#define P(a,b,c,d,x,s) \ + do \ + { \ (a) += F((b),(c),(d)) + (x); \ (a) = S((a),(s)); \ } while( 0 ) @@ -177,8 +177,8 @@ int mbedtls_internal_md4_process( mbedtls_md4_context *ctx, #define P(a,b,c,d,x,s) \ do \ { \ - (a) += F(b,c,d) + (x) + 0x5A827999; \ - (a) = S(a,s); \ + (a) += F((b),(c),(d)) + (x) + 0x5A827999; \ + (a) = S((a),(s)); \ } while( 0 ) P( A, B, C, D, X[ 0], 3 ); @@ -202,11 +202,11 @@ int mbedtls_internal_md4_process( mbedtls_md4_context *ctx, #undef F #define F(x,y,z) ((x) ^ (y) ^ (z)) -#define P(a,b,c,d,x,s) \ - do \ - { \ - (a) += F(b,c,d) + (x) + 0x6ED9EBA1; \ - (a) = S(a,s); \ +#define P(a,b,c,d,x,s) \ + do \ + { \ + (a) += F((b),(c),(d)) + (x) + 0x6ED9EBA1; \ + (a) = S((a),(s)); \ } while( 0 ) P( A, B, C, D, X[ 0], 3 ); diff --git a/library/sha512.c b/library/sha512.c index efc4acb40..bdd20b284 100644 --- a/library/sha512.c +++ b/library/sha512.c @@ -225,7 +225,7 @@ int mbedtls_internal_sha512_process( mbedtls_sha512_context *ctx, SHA512_VALIDATE_RET( (const unsigned char *)data != NULL ); #define SHR(x,n) ((x) >> (n)) -#define ROTR(x,n) (SHR(x,n) | ((x) << (64 - (n)))) +#define ROTR(x,n) (SHR((x),(n)) | ((x) << (64 - (n)))) #define S0(x) (ROTR(x, 1) ^ ROTR(x, 8) ^ SHR(x, 7)) #define S1(x) (ROTR(x,19) ^ ROTR(x,61) ^ SHR(x, 6)) @@ -236,12 +236,12 @@ int mbedtls_internal_sha512_process( mbedtls_sha512_context *ctx, #define F0(x,y,z) (((x) & (y)) | ((z) & ((x) | (y)))) #define F1(x,y,z) ((z) ^ ((x) & ((y) ^ (z)))) -#define P(a,b,c,d,e,f,g,h,x,K) \ - do \ - { \ +#define P(a,b,c,d,e,f,g,h,x,K) \ + do \ + { \ temp1 = (h) + S3(e) + F1((e),(f),(g)) + (K) + (x); \ temp2 = S2(a) + F0((a),(b),(c)); \ - (d) += temp1; (h) = temp1 + temp2; \ + (d) += temp1; (h) = temp1 + temp2; \ } while( 0 ) for( i = 0; i < 16; i++ )