From 5729d4e4a8ddb9eb391bfbc7ee0c2486310be1d9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 7 Sep 2021 20:02:50 +0200 Subject: [PATCH 01/16] WIP report on structure types Report the size, alignment and offset of fields of structures defined in Mbed TLS headers. Usage example on Cortex-M0: ``` cd mbedtls/struct-optimization scripts/config.py baremetal ../types-report/scripts/types_report.py -t armv6m-unknown-eabi -I ~/Packages/ARMCompiler6.6/include ``` Signed-off-by: Gilles Peskine --- scripts/types_report.py | 232 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 232 insertions(+) create mode 100755 scripts/types_report.py diff --git a/scripts/types_report.py b/scripts/types_report.py new file mode 100755 index 0000000000..0b0c972e0b --- /dev/null +++ b/scripts/types_report.py @@ -0,0 +1,232 @@ +#!/usr/bin/env python3 +"""Report on structure types defined in Mbed TLS headers. + +This script uses the clang python bindings (``pip3 install --user clang``). +""" + +# Copyright The Mbed TLS Contributors +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import argparse +import collections +import enum +import glob +import os +import re +import sys +from typing import Dict, FrozenSet, List, Optional + +import clang.cindex #type: ignore +from clang.cindex import Cursor, SourceLocation, TranslationUnit +from clang.cindex import CursorKind, TypeKind + +from mbedtls_dev import typing_util + + +class Kind(enum.Enum): + UNKNOWN = 0 + POINTER = 1 + INTEGER = 2 + ENUM = 3 + STRUCT = 4 + UNION = 5 + + +class Type: + """Information about a type defined by the library.""" + + def __init__(self, name: str, kind: Kind) -> None: + self.name = name + self.kind = kind + + +class Struct(Type): + """Information about a structure type.""" + + def __init__(self, name: str, fields: List[str]) -> None: + super().__init__(name, Kind.STRUCT) + self.fields = fields + +class Union(Type): + """Information about a union type.""" + + def __init__(self, name: str, fields: List[str]) -> None: + super().__init__(name, Kind.UNION) + self.fields = fields + + +class Ast: + """Abstract representation of the source code.""" + + def __init__(self, options) -> None: + """Prepare for analysis of C source files.""" + if options.clang_library_file: + clang.cindex.Config.set_library_file(options.clang_library_file) + self.parse_options = [] + if options.target: + self.parse_options += ['-target', options.target] + for d in options.include: + self.parse_options.append('-I' + d) + for d in options.define: + self.parse_options.append('-D' + d) + self.index = clang.cindex.Index.create() + self.files = {} #type: Dict[str, TranslationUnit] + self.types = {} #type: Dict[str, Type] + self.typedefs = {} #type: Dict[str, Cursor] + self.structs = {} #type: Dict[str, Cursor] + # fields[TYPE_OR_STRUCT_NAME][FIELD_NAME] + self.fields = {} #type: Dict[str, Dict[str, Cursor]] + + def load(self, *filenames: str) -> None: + """Load the AST of the given source files.""" + for filename in filenames: + self.files[filename] = self.index.parse(filename, + self.parse_options) + + INTERESTING_FILE_RE = re.compile(r'(?:.*/)?(mbedtls|psa)/[^/]*\.h\Z') + def in_interesting_file(self, location: SourceLocation) -> bool: + """Whether the given location is in a file that should be analyzed. + + This function detects Mbed TLS headers. + """ + if not hasattr(location.file, 'name'): + # Some artificial nodes have associated no file name. + # Let's hope they're not important. + return True + filename = location.file.name + if self.INTERESTING_FILE_RE.match(filename): + return True + return False + + def record_typedef(self, node: Cursor) -> None: + """Record information about a type definition.""" + actual = node + while hasattr(actual, 'underlying_typedef_type'): + actual = actual.underlying_typedef_type + kind = Kind.UNKNOWN + if actual.kind == TypeKind.ELABORATED and \ + actual.spelling.startswith('struct '): + kind = Kind.STRUCT + fields = [field.spelling for field in actual.get_fields()] + data = Struct(node.spelling, fields) + else: + data = Type(node.spelling, kind) + self.types[node.spelling] = data + self.typedefs[node.spelling] = actual + + def read_type_definitions(self, filenames: List[str]) -> None: + """Parse type definitions in the given C source files (usually headers).""" + self.load(*filenames) + for filename in filenames: + for node in self.files[filename].cursor.walk_preorder(): + if not self.in_interesting_file(node.location): + continue + if node.kind == CursorKind.TYPEDEF_DECL: + self.record_typedef(node) + # elif node.kind == CursorKind.STRUCT_DECL: + # # print(node.spelling, node.location, node.underlying_typedef_type, list(node.underlying_typedef_type.get_fields())) + # # import pdb; pdb.set_trace() + # self.structs[node.spelling] = node.underlying_typedef_type + elif node.kind == CursorKind.FIELD_DECL: + # If node.lexical_parent.spelling is an empty string, + # the field is inside an anonymous structure nested in + # another structure. + type_name = node.lexical_parent.spelling + if not type_name: + continue + self.fields.setdefault(type_name, collections.OrderedDict()) + self.fields[type_name][node.spelling] = node + + def report_type(self, out: typing_util.Writable, + name: str, node: Cursor) -> None: + """Print information about a type. + + Format: ,,,, + """ + kind = re.sub(r'.*\.', r'', str(node.kind)) + out.write('{},{},{},{},\n'.format( + name, kind, node.get_size(), node.get_align() + )) + + def report_types(self, out: typing_util.Writable) -> None: + """Print information about types defined by the library. + """ + for name in sorted(self.typedefs): + node = self.typedefs[name] + self.report_type(out, name, node) + + def report_field(self, out: typing_util.Writable, + prefix: str, field: Cursor) -> None: + """Print information about a structure field. + + Format: .,"FIELD",,, + """ + out.write('{},FIELD,{},{},{}\n'.format( + prefix + field.spelling, + field.type.get_size(), field.type.get_align(), + field.get_field_offsetof() + )) + + def report_fields_from_typedefs(self, out: typing_util.Writable) -> None: + """Print information about fields of structures defined by the library. + + This implementation only enumerates fields from structures defined + in typedefs, not fields defined in separate struct definitions. + """ + for name in sorted(self.typedefs): + node = self.typedefs[name] + if hasattr(node, 'get_fields'): + for field in sorted(node.get_fields(), + key=lambda f: f.spelling): + self.report_field(out, name + '.', field) + + def report_fields(self, out: typing_util.Writable) -> None: + """Print information about fields of structures defined by the library.""" + for type_name in sorted(self.fields): + for field in self.fields[type_name].values(): + self.report_field(out, type_name + '.', field) + + @staticmethod + def header_files() -> List[str]: + return [filename + for pat in ['include/*/*.h', 'library/*.h'] + for filename in sorted(glob.glob(pat))] + + def run_analysis(self) -> None: + self.read_type_definitions(self.header_files()) + self.report_types(sys.stdout) + self.report_fields(sys.stdout) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('--clang-library-file', + help="Alternative location of libclang.so") + parser.add_argument('--define', '-D', + action='append', + default=['MBEDTLS_ALLOW_PRIVATE_ACCESS'], + help="Additional C preprocessor definition") + parser.add_argument('--include', '-I', + action='append', + default=['include'], + help="Directory to add to the header include path") + parser.add_argument('--target', '-t', + help="Target triple to build for (default: native build)") + options = parser.parse_args() + ast = Ast(options) + ast.run_analysis() + +if __name__ == '__main__': + main() From ccc1f808208af1f041c5c08d9ccdf4b522242f47 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Tue, 7 Sep 2021 20:18:03 +0200 Subject: [PATCH 02/16] offsetof is in bits, not bytes Signed-off-by: Gilles Peskine --- scripts/types_report.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts/types_report.py b/scripts/types_report.py index 0b0c972e0b..9cbc355ff8 100755 --- a/scripts/types_report.py +++ b/scripts/types_report.py @@ -173,10 +173,14 @@ class Ast: Format: .,"FIELD",,, """ + # Empirically, offsetof is in bits, not bytes. To make the output + # easier to read, convert to bytes (the same unit as size and + # alignment), which means that bitfields will be located at their + # first byte. + offset = field.get_field_offsetof() // 8 out.write('{},FIELD,{},{},{}\n'.format( prefix + field.spelling, - field.type.get_size(), field.type.get_align(), - field.get_field_offsetof() + field.type.get_size(), field.type.get_align(), offset )) def report_fields_from_typedefs(self, out: typing_util.Writable) -> None: From d8ef8d41de933527771c10a93db274bd305fe317 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Sep 2021 22:53:24 +0200 Subject: [PATCH 03/16] Remove type definition parsing and reporting This script is focusing on fields. Signed-off-by: Gilles Peskine --- scripts/types_report.py | 98 ++--------------------------------------- 1 file changed, 4 insertions(+), 94 deletions(-) diff --git a/scripts/types_report.py b/scripts/types_report.py index 9cbc355ff8..687f03ff20 100755 --- a/scripts/types_report.py +++ b/scripts/types_report.py @@ -21,7 +21,6 @@ This script uses the clang python bindings (``pip3 install --user clang``). import argparse import collections -import enum import glob import os import re @@ -35,38 +34,6 @@ from clang.cindex import CursorKind, TypeKind from mbedtls_dev import typing_util -class Kind(enum.Enum): - UNKNOWN = 0 - POINTER = 1 - INTEGER = 2 - ENUM = 3 - STRUCT = 4 - UNION = 5 - - -class Type: - """Information about a type defined by the library.""" - - def __init__(self, name: str, kind: Kind) -> None: - self.name = name - self.kind = kind - - -class Struct(Type): - """Information about a structure type.""" - - def __init__(self, name: str, fields: List[str]) -> None: - super().__init__(name, Kind.STRUCT) - self.fields = fields - -class Union(Type): - """Information about a union type.""" - - def __init__(self, name: str, fields: List[str]) -> None: - super().__init__(name, Kind.UNION) - self.fields = fields - - class Ast: """Abstract representation of the source code.""" @@ -83,9 +50,6 @@ class Ast: self.parse_options.append('-D' + d) self.index = clang.cindex.Index.create() self.files = {} #type: Dict[str, TranslationUnit] - self.types = {} #type: Dict[str, Type] - self.typedefs = {} #type: Dict[str, Cursor] - self.structs = {} #type: Dict[str, Cursor] # fields[TYPE_OR_STRUCT_NAME][FIELD_NAME] self.fields = {} #type: Dict[str, Dict[str, Cursor]] @@ -110,36 +74,14 @@ class Ast: return True return False - def record_typedef(self, node: Cursor) -> None: - """Record information about a type definition.""" - actual = node - while hasattr(actual, 'underlying_typedef_type'): - actual = actual.underlying_typedef_type - kind = Kind.UNKNOWN - if actual.kind == TypeKind.ELABORATED and \ - actual.spelling.startswith('struct '): - kind = Kind.STRUCT - fields = [field.spelling for field in actual.get_fields()] - data = Struct(node.spelling, fields) - else: - data = Type(node.spelling, kind) - self.types[node.spelling] = data - self.typedefs[node.spelling] = actual - - def read_type_definitions(self, filenames: List[str]) -> None: - """Parse type definitions in the given C source files (usually headers).""" + def read_field_definitions(self, filenames: List[str]) -> None: + """Parse structure field definitions in the given C source files (usually headers).""" self.load(*filenames) for filename in filenames: for node in self.files[filename].cursor.walk_preorder(): if not self.in_interesting_file(node.location): continue - if node.kind == CursorKind.TYPEDEF_DECL: - self.record_typedef(node) - # elif node.kind == CursorKind.STRUCT_DECL: - # # print(node.spelling, node.location, node.underlying_typedef_type, list(node.underlying_typedef_type.get_fields())) - # # import pdb; pdb.set_trace() - # self.structs[node.spelling] = node.underlying_typedef_type - elif node.kind == CursorKind.FIELD_DECL: + if node.kind == CursorKind.FIELD_DECL: # If node.lexical_parent.spelling is an empty string, # the field is inside an anonymous structure nested in # another structure. @@ -149,24 +91,6 @@ class Ast: self.fields.setdefault(type_name, collections.OrderedDict()) self.fields[type_name][node.spelling] = node - def report_type(self, out: typing_util.Writable, - name: str, node: Cursor) -> None: - """Print information about a type. - - Format: ,,,, - """ - kind = re.sub(r'.*\.', r'', str(node.kind)) - out.write('{},{},{},{},\n'.format( - name, kind, node.get_size(), node.get_align() - )) - - def report_types(self, out: typing_util.Writable) -> None: - """Print information about types defined by the library. - """ - for name in sorted(self.typedefs): - node = self.typedefs[name] - self.report_type(out, name, node) - def report_field(self, out: typing_util.Writable, prefix: str, field: Cursor) -> None: """Print information about a structure field. @@ -183,19 +107,6 @@ class Ast: field.type.get_size(), field.type.get_align(), offset )) - def report_fields_from_typedefs(self, out: typing_util.Writable) -> None: - """Print information about fields of structures defined by the library. - - This implementation only enumerates fields from structures defined - in typedefs, not fields defined in separate struct definitions. - """ - for name in sorted(self.typedefs): - node = self.typedefs[name] - if hasattr(node, 'get_fields'): - for field in sorted(node.get_fields(), - key=lambda f: f.spelling): - self.report_field(out, name + '.', field) - def report_fields(self, out: typing_util.Writable) -> None: """Print information about fields of structures defined by the library.""" for type_name in sorted(self.fields): @@ -209,8 +120,7 @@ class Ast: for filename in sorted(glob.glob(pat))] def run_analysis(self) -> None: - self.read_type_definitions(self.header_files()) - self.report_types(sys.stdout) + self.read_field_definitions(self.header_files()) self.report_fields(sys.stdout) From 6904f70b8d38a3afe722eb22a96118764f323e03 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Sep 2021 22:54:19 +0200 Subject: [PATCH 04/16] More accurate script name This script is focusing on fields. Signed-off-by: Gilles Peskine --- scripts/{types_report.py => field_report.py} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename scripts/{types_report.py => field_report.py} (100%) diff --git a/scripts/types_report.py b/scripts/field_report.py similarity index 100% rename from scripts/types_report.py rename to scripts/field_report.py From ff081597c11b050f9051b0b30bf79500b13fca18 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Sep 2021 22:56:10 +0200 Subject: [PATCH 05/16] Remove useless output column Signed-off-by: Gilles Peskine --- scripts/field_report.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 687f03ff20..00b7d25e71 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -95,14 +95,14 @@ class Ast: prefix: str, field: Cursor) -> None: """Print information about a structure field. - Format: .,"FIELD",,, + Format: .,,, """ # Empirically, offsetof is in bits, not bytes. To make the output # easier to read, convert to bytes (the same unit as size and # alignment), which means that bitfields will be located at their # first byte. offset = field.get_field_offsetof() // 8 - out.write('{},FIELD,{},{},{}\n'.format( + out.write('{},{},{},{}\n'.format( prefix + field.spelling, field.type.get_size(), field.type.get_align(), offset )) From bdeb420cc7276fac20a8160b7a8f3d1eaaff5607 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Sep 2021 23:20:57 +0200 Subject: [PATCH 06/16] Count accesses to each field Report how many places in the code access each field. Known bug: the code in this commit doesn't match type names at the point of declaration with type names at the point of use properly. I think the problem is that field definitions are recorded against the name of the struct when it has one, but field uses are recorded against the typedef. Signed-off-by: Gilles Peskine --- scripts/field_report.py | 75 +++++++++++++++++++++++++++++++++++------ 1 file changed, 64 insertions(+), 11 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 00b7d25e71..648437635e 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -34,6 +34,17 @@ from clang.cindex import CursorKind, TypeKind from mbedtls_dev import typing_util +class FieldInfo: + """Information about a field of a structure.""" + + def __init__(self, node: Cursor) -> None: + self.node = node + self.uses = 0 + + def record_use(self, _node: Cursor) -> None: + self.uses += 1 + + class Ast: """Abstract representation of the source code.""" @@ -51,7 +62,7 @@ class Ast: self.index = clang.cindex.Index.create() self.files = {} #type: Dict[str, TranslationUnit] # fields[TYPE_OR_STRUCT_NAME][FIELD_NAME] - self.fields = {} #type: Dict[str, Dict[str, Cursor]] + self.fields = {} #type: Dict[str, Dict[str, FieldInfo]] def load(self, *filenames: str) -> None: """Load the AST of the given source files.""" @@ -59,11 +70,11 @@ class Ast: self.files[filename] = self.index.parse(filename, self.parse_options) - INTERESTING_FILE_RE = re.compile(r'(?:.*/)?(mbedtls|psa)/[^/]*\.h\Z') + INTERESTING_FILE_RE = re.compile(r'(?:.*/)?(mbedtls|psa|library)/[^/]*\.[ch]\Z') def in_interesting_file(self, location: SourceLocation) -> bool: """Whether the given location is in a file that should be analyzed. - This function detects Mbed TLS headers. + This function detects Mbed TLS headers and source files. """ if not hasattr(location.file, 'name'): # Some artificial nodes have associated no file name. @@ -89,22 +100,57 @@ class Ast: if not type_name: continue self.fields.setdefault(type_name, collections.OrderedDict()) - self.fields[type_name][node.spelling] = node + self.fields[type_name][node.spelling] = FieldInfo(node) + + QUALIFIERS_RE = re.compile(r'\s*(const|volatile|restrict)\s+') + def get_type_core(self, typ: clang.cindex.Type) -> str: + """Get the base name of a type, without typedefs, qualifiers or pointers.""" + while typ.kind == TypeKind.POINTER: + typ = typ.get_pointee() + while hasattr(typ, 'underlying_typedef_type'): + typ = typ.underlying_typedef_type + while typ.kind == TypeKind.POINTER: + typ = typ.get_pointee() + # There's no API function to remove qualifiers from a type, + # so do it textually. + return re.sub(self.QUALIFIERS_RE, r'', typ.spelling) + + def record_field_access(self, node: Cursor) -> None: + """Record one location where a field is accessed.""" + field_name = node.spelling + lhs = next(node.get_children()) + structure_type = self.get_type_core(lhs.type) + if structure_type not in self.fields: + # This is not a structure defined by the library + return + self.fields[structure_type][field_name].record_use(node) + + def read_field_usage(self, filenames: List[str]) -> None: + """Parse field usage in the given C source files.""" + self.load(*filenames) + for filename in filenames: + for node in self.files[filename].cursor.walk_preorder(): + if not self.in_interesting_file(node.location): + continue + if node.kind == CursorKind.MEMBER_REF_EXPR: + self.record_field_access(node) def report_field(self, out: typing_util.Writable, - prefix: str, field: Cursor) -> None: + prefix: str, field: FieldInfo) -> None: """Print information about a structure field. - Format: .,,, + Format: .,,,,use_count """ + type_node = field.node.type # Empirically, offsetof is in bits, not bytes. To make the output # easier to read, convert to bytes (the same unit as size and # alignment), which means that bitfields will be located at their # first byte. - offset = field.get_field_offsetof() // 8 - out.write('{},{},{},{}\n'.format( - prefix + field.spelling, - field.type.get_size(), field.type.get_align(), offset + offset = field.node.get_field_offsetof() // 8 + out.write('{},{},{},{},{}\n'.format( + prefix + field.node.spelling, + type_node.get_size(), type_node.get_align(), offset, + field.uses )) def report_fields(self, out: typing_util.Writable) -> None: @@ -119,8 +165,15 @@ class Ast: for pat in ['include/*/*.h', 'library/*.h'] for filename in sorted(glob.glob(pat))] + @staticmethod + def c_files() -> List[str]: + return sorted(glob.glob('library/*.c')) + def run_analysis(self) -> None: - self.read_field_definitions(self.header_files()) + header_files = self.header_files() + c_files = self.c_files() + self.read_field_definitions(header_files) + self.read_field_usage(header_files + c_files) self.report_fields(sys.stdout) From d8a174c005bf0354c98f0fb9f8b3c77e27de2a90 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Sep 2021 18:36:14 +0200 Subject: [PATCH 07/16] Ignore AST nodes with an unknown location The source code said this was done, but the code did the opposite. Signed-off-by: Gilles Peskine --- scripts/field_report.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 648437635e..4a02ae2b2e 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -79,7 +79,7 @@ class Ast: if not hasattr(location.file, 'name'): # Some artificial nodes have associated no file name. # Let's hope they're not important. - return True + return False filename = location.file.name if self.INTERESTING_FILE_RE.match(filename): return True From 5d27290f4eadb2036b94d13fae9507dc90920cc5 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Sep 2021 18:56:21 +0200 Subject: [PATCH 08/16] See through typedefs when parsing field usage The previous code did not correctly associate typedefs with structs when the two had different names. Signed-off-by: Gilles Peskine --- scripts/field_report.py | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 4a02ae2b2e..4524ef15e2 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -102,18 +102,36 @@ class Ast: self.fields.setdefault(type_name, collections.OrderedDict()) self.fields[type_name][node.spelling] = FieldInfo(node) - QUALIFIERS_RE = re.compile(r'\s*(const|volatile|restrict)\s+') - def get_type_core(self, typ: clang.cindex.Type) -> str: + @staticmethod + def get_underlying_type(typ: clang.cindex.Type) -> Optional[clang.cindex.Type]: + """Strip off one level of type indirection. + + Return None if the type is as primitive as can be. + """ + if hasattr(typ, 'get_canonical'): + lower = typ.get_canonical() + if lower == typ: + return None + else: + return lower + if typ.kind == TypeKind.POINTER: + return typ.get_pointee() + if hasattr(typ, 'underlying_typedef_type'): + return typ.underlying_typedef_type + return None + + QUALIFIERS_RE = re.compile(r'.* ') + def get_type_core(self, type: clang.cindex.Type) -> str: """Get the base name of a type, without typedefs, qualifiers or pointers.""" - while typ.kind == TypeKind.POINTER: - typ = typ.get_pointee() - while hasattr(typ, 'underlying_typedef_type'): - typ = typ.underlying_typedef_type - while typ.kind == TypeKind.POINTER: - typ = typ.get_pointee() + core = type + lower = type # type: Optional[clang.cindex.Type] + while lower: + core, lower = lower, self.get_underlying_type(core) # There's no API function to remove qualifiers from a type, - # so do it textually. - return re.sub(self.QUALIFIERS_RE, r'', typ.spelling) + # so do it textually. Remove 'const', 'restrict', etc. + # Also remove 'struct', so we'll get the struct name from struct + # definitions. + return re.sub(self.QUALIFIERS_RE, r'', core.spelling) def record_field_access(self, node: Cursor) -> None: """Record one location where a field is accessed.""" From 9d918e70919920faeb7c3ee23f56b04e51decd2c Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Sep 2021 19:02:21 +0200 Subject: [PATCH 09/16] Act on files passed on the command line, not hardcoded mbedtls files This is a lot easier for testing. Production usage is now ``` scripts/field_report.py include/*/*.h library/*.[hc] ``` Signed-off-by: Gilles Peskine --- scripts/field_report.py | 35 ++++++++--------------------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 4524ef15e2..bddff712a2 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -21,8 +21,6 @@ This script uses the clang python bindings (``pip3 install --user clang``). import argparse import collections -import glob -import os import re import sys from typing import Dict, FrozenSet, List, Optional @@ -70,20 +68,13 @@ class Ast: self.files[filename] = self.index.parse(filename, self.parse_options) - INTERESTING_FILE_RE = re.compile(r'(?:.*/)?(mbedtls|psa|library)/[^/]*\.[ch]\Z') def in_interesting_file(self, location: SourceLocation) -> bool: - """Whether the given location is in a file that should be analyzed. - - This function detects Mbed TLS headers and source files. - """ + """Whether the given location is in a file that should be analyzed.""" if not hasattr(location.file, 'name'): # Some artificial nodes have associated no file name. # Let's hope they're not important. return False - filename = location.file.name - if self.INTERESTING_FILE_RE.match(filename): - return True - return False + return location.file.name in self.files def read_field_definitions(self, filenames: List[str]) -> None: """Parse structure field definitions in the given C source files (usually headers).""" @@ -177,21 +168,9 @@ class Ast: for field in self.fields[type_name].values(): self.report_field(out, type_name + '.', field) - @staticmethod - def header_files() -> List[str]: - return [filename - for pat in ['include/*/*.h', 'library/*.h'] - for filename in sorted(glob.glob(pat))] - - @staticmethod - def c_files() -> List[str]: - return sorted(glob.glob('library/*.c')) - - def run_analysis(self) -> None: - header_files = self.header_files() - c_files = self.c_files() - self.read_field_definitions(header_files) - self.read_field_usage(header_files + c_files) + def run_analysis(self, files) -> None: + self.read_field_definitions(files) + self.read_field_usage(files) self.report_fields(sys.stdout) @@ -209,9 +188,11 @@ def main(): help="Directory to add to the header include path") parser.add_argument('--target', '-t', help="Target triple to build for (default: native build)") + parser.add_argument('files', metavar='FILE', nargs='*', + help="Source files to analyze") options = parser.parse_args() ast = Ast(options) - ast.run_analysis() + ast.run_analysis(options.files) if __name__ == '__main__': main() From 5336375090ee2fc623e367110ac0b1cb80a5c5dc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Sep 2021 19:08:54 +0200 Subject: [PATCH 10/16] Code cleanups No behavior change. Signed-off-by: Gilles Peskine --- scripts/field_report.py | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index bddff712a2..1dc4db248f 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -23,7 +23,7 @@ import argparse import collections import re import sys -from typing import Dict, FrozenSet, List, Optional +from typing import Dict, List, Optional import clang.cindex #type: ignore from clang.cindex import Cursor, SourceLocation, TranslationUnit @@ -37,10 +37,20 @@ class FieldInfo: def __init__(self, node: Cursor) -> None: self.node = node - self.uses = 0 + self.lexical_uses = 0 + self.size = node.type.get_size() + self.align = node.type.get_align() + # Empirically, offsetof is in bits, not bytes. To make the output + # easier to read, convert to bytes (the same unit as size and + # alignment), which means that bitfields will be located at their + # first byte. + self.offset = node.get_field_offsetof() // 8 def record_use(self, _node: Cursor) -> None: - self.uses += 1 + self.lexical_uses += 1 + + def uses(self) -> int: + return self.lexical_uses class Ast: @@ -113,6 +123,7 @@ class Ast: QUALIFIERS_RE = re.compile(r'.* ') def get_type_core(self, type: clang.cindex.Type) -> str: + # pylint: disable=redefined-builtin """Get the base name of a type, without typedefs, qualifiers or pointers.""" core = type lower = type # type: Optional[clang.cindex.Type] @@ -144,22 +155,17 @@ class Ast: if node.kind == CursorKind.MEMBER_REF_EXPR: self.record_field_access(node) - def report_field(self, out: typing_util.Writable, + @staticmethod + def report_field(out: typing_util.Writable, prefix: str, field: FieldInfo) -> None: """Print information about a structure field. Format: .,,,,use_count """ - type_node = field.node.type - # Empirically, offsetof is in bits, not bytes. To make the output - # easier to read, convert to bytes (the same unit as size and - # alignment), which means that bitfields will be located at their - # first byte. - offset = field.node.get_field_offsetof() // 8 out.write('{},{},{},{},{}\n'.format( prefix + field.node.spelling, - type_node.get_size(), type_node.get_align(), offset, - field.uses + field.size, field.align, field.offset, + field.uses() )) def report_fields(self, out: typing_util.Writable) -> None: From 53a581fe0a8dfea4e7126eb8b2a01b8eddfb8cb0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Sep 2021 20:03:31 +0200 Subject: [PATCH 11/16] Don't skip typedef struct {...} Don't skip structure definitions that have no name, but for which there is a named typedef. Do skip unions and anonymous structs that don't have a type name (which is common for structs embedded in another struct or union). Signed-off-by: Gilles Peskine --- scripts/field_report.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 1dc4db248f..a6fe932b6d 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -94,11 +94,17 @@ class Ast: if not self.in_interesting_file(node.location): continue if node.kind == CursorKind.FIELD_DECL: - # If node.lexical_parent.spelling is an empty string, - # the field is inside an anonymous structure nested in - # another structure. type_name = node.lexical_parent.spelling + # Skip unions + if node.lexical_parent.kind != CursorKind.STRUCT_DECL: + continue + # type_name is the struct name. If there's no struct + # name, see if there's a typedef name, to cope with + # "typedef struct { ... } foo;" if not type_name: + type_name = node.lexical_parent.type.spelling + # Skip anonymous structs for now. + if '(anonymous ' in type_name: continue self.fields.setdefault(type_name, collections.OrderedDict()) self.fields[type_name][node.spelling] = FieldInfo(node) From 766ff6802e99080c2336bb8abfac9a4db897b7ee Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 16 Sep 2021 20:04:49 +0200 Subject: [PATCH 12/16] Calculate a score for fields The score is the access cost on Cortex-M0+ thumb builds, expressed in instructions. This commit does not have any provisions to calculate different scores based on the target architecture, so it's only meaningful when running this script with `-t armv6m-unknown-eabi`. Go with a simple model: direct access is possible for the first 128 fields of a structure, direct access costs 1 instruction, indirect access costs 3 instructions. The score calculation assumes that each lexical use in the source code corresponds to one access. This does not take into account unused code, inlining, uses from application code, etc. Signed-off-by: Gilles Peskine --- scripts/field_report.py | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index a6fe932b6d..858c24f125 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -52,6 +52,21 @@ class FieldInfo: def uses(self) -> int: return self.lexical_uses + def is_indirect(self) -> bool: + """Whether access to this field is indirect on ARM Cortex-M0+.""" + word_offset = self.offset // self.align + return word_offset >= 128 + + def score(self) -> int: + """A field's score is an estimate of its access cost. + + The cost is calculated as the number of instructions needed to + access the field on ARM Cortex-M0+, multiplied by the number of uses + of the field inside the library. + """ + cost_per_use = 1 + 2 * self.is_indirect() + return self.uses() * cost_per_use + class Ast: """Abstract representation of the source code.""" @@ -166,12 +181,12 @@ class Ast: prefix: str, field: FieldInfo) -> None: """Print information about a structure field. - Format: .,,,,use_count + Format: .,,,,, """ - out.write('{},{},{},{},{}\n'.format( + out.write('{},{},{},{},{},{}\n'.format( prefix + field.node.spelling, field.size, field.align, field.offset, - field.uses() + field.uses(), field.score() )) def report_fields(self, out: typing_util.Writable) -> None: From 3cab86e117f60f1be5150cfbef36a54009b52396 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 17 Sep 2021 21:28:42 +0200 Subject: [PATCH 13/16] Don't stop get_type_core at a canonical type get_canonical() sees through typedefs, but it doesn't dereference pointers. So we need to continue digging after reaching a canonical type. Signed-off-by: Gilles Peskine --- scripts/field_report.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 858c24f125..5e794c68ce 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -132,9 +132,7 @@ class Ast: """ if hasattr(typ, 'get_canonical'): lower = typ.get_canonical() - if lower == typ: - return None - else: + if lower != typ: return lower if typ.kind == TypeKind.POINTER: return typ.get_pointee() From 0b2d97736b547809bf1221c9031b0f31a54b7876 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 17 Sep 2021 21:30:07 +0200 Subject: [PATCH 14/16] Field report driver script Remove the remaining Mbed TLS-specific bits of field_report.py. Create a wrapper script that calls field_report.py with options for Mbed TLS specifically. Signed-off-by: Gilles Peskine --- scripts/field_report.py | 4 ++-- scripts/field_report.sh | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 2 deletions(-) create mode 100755 scripts/field_report.sh diff --git a/scripts/field_report.py b/scripts/field_report.py index 5e794c68ce..55839207ea 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -205,11 +205,11 @@ def main(): help="Alternative location of libclang.so") parser.add_argument('--define', '-D', action='append', - default=['MBEDTLS_ALLOW_PRIVATE_ACCESS'], + default=[], help="Additional C preprocessor definition") parser.add_argument('--include', '-I', action='append', - default=['include'], + default=[], help="Directory to add to the header include path") parser.add_argument('--target', '-t', help="Target triple to build for (default: native build)") diff --git a/scripts/field_report.sh b/scripts/field_report.sh new file mode 100755 index 0000000000..a6bfc52f29 --- /dev/null +++ b/scripts/field_report.sh @@ -0,0 +1,18 @@ +#!/bin/sh + +# Usage example: +# scripts/field_report.sh -t armv6m-unknown-eabi -I build-m0plus/include -I ~/Packages/ARMCompiler6.6/include >fields-baremetal-m0plus.csv + +set -eu + +script_dir=$(dirname -- "$0") +lib_dir=. +if [ ! -d "$lib_dir/library" ] && [ -d "${lib_dir%/*}/library" ]; then + lib_dir=${lib_dir%/*} +fi + +"$script_dir/field_report.py" \ + -DMBEDTLS_ALLOW_PRIVATE_ACCESS \ + "$@" \ + -I "$lib_dir/include" \ + "$lib_dir/include"/*/*.h "$lib_dir/library"/*.[hc] From 6c62227cd9422a477664dba9c8b47253fb0ce2b6 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 18 Nov 2021 17:26:20 +0100 Subject: [PATCH 15/16] Improve usability: documentation, sanity checks Add a sanity check to detect the most common problem, which is a missing include directory leading to compilation errors (which this script is unable to report due to a limitation of the clang python bindings). Signed-off-by: Gilles Peskine --- scripts/field_report.py | 53 +++++++++++++++++++++++++++++++++++------ 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 55839207ea..8d7c3e0944 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -1,7 +1,13 @@ #!/usr/bin/env python3 -"""Report on structure types defined in Mbed TLS headers. +"""Report on structure types defined in C source files. This script uses the clang python bindings (``pip3 install --user clang``). + +This script only works on code that compiles. If there are any errors, +this script will just return garbage data (typically missing fields, or +having 0 values everywhere). The most common cause of failure is missing +include directories, either for the code you're analyzing or for the +standard library when cross-compiling. """ # Copyright The Mbed TLS Contributors @@ -32,6 +38,14 @@ from clang.cindex import CursorKind, TypeKind from mbedtls_dev import typing_util +class SanityCheck(Exception): + def __init__(self, msg: str) -> None: + super().__init__('Sanity check failed: ' + msg + + '\nThis likely indicates a compilation error.' + + '\nMaybe a missing include directory (-I)?') + + + class FieldInfo: """Information about a field of a structure.""" @@ -164,6 +178,21 @@ class Ast: return self.fields[structure_type][field_name].record_use(node) + def sanity_checks(self) -> None: + """If the data looks wrong, raise a `SanityCheck` exception.""" + for type_name in sorted(self.fields): + for field in self.fields[type_name].values(): + if field.offset == -1: + raise SanityCheck('Could not determine offset of {}.{}. ' + .format(type_name, field)) + + def run_analysis(self, files: List[str], + sanity_checks: bool = True) -> None: + self.read_field_definitions(files) + self.read_field_usage(files) + if sanity_checks: + self.sanity_checks() + def read_field_usage(self, filenames: List[str]) -> None: """Parse field usage in the given C source files.""" self.load(*filenames) @@ -187,16 +216,19 @@ class Ast: field.uses(), field.score() )) - def report_fields(self, out: typing_util.Writable) -> None: + def report_fields(self, out: typing_util.Writable, + header=False) -> None: """Print information about fields of structures defined by the library.""" + if header: + out.write('field,size,align,offset,uses,score\n') for type_name in sorted(self.fields): for field in self.fields[type_name].values(): self.report_field(out, type_name + '.', field) - def run_analysis(self, files) -> None: - self.read_field_definitions(files) - self.read_field_usage(files) - self.report_fields(sys.stdout) + def report(self, options, out: typing_util.Writable) -> None: + """Report what this script has to report.""" + self.report_fields(out, + header=options.csv_header) def main(): @@ -211,13 +243,20 @@ def main(): action='append', default=[], help="Directory to add to the header include path") + parser.add_argument('--no-csv-header', + dest='csv_header', default=True, action='store_false', + help="Omit the CSV header from the output") + parser.add_argument('--no-sanity-checks', + dest='sanity_checks', default=True, action='store_false', + help="Omit sanity checks, print output even if it's suspicious") parser.add_argument('--target', '-t', help="Target triple to build for (default: native build)") parser.add_argument('files', metavar='FILE', nargs='*', help="Source files to analyze") options = parser.parse_args() ast = Ast(options) - ast.run_analysis(options.files) + ast.run_analysis(options.files, sanity_checks=options.sanity_checks) + ast.report(options, sys.stdout) if __name__ == '__main__': main() From 1fbf373e3dc3d83d4bc56febff8cabe5a34c78cc Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Thu, 25 Nov 2021 19:46:47 +0100 Subject: [PATCH 16/16] With --no-sanity-checks, do run the checks, but produce output anyway Signed-off-by: Gilles Peskine --- scripts/field_report.py | 44 ++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/scripts/field_report.py b/scripts/field_report.py index 8d7c3e0944..bae0f9c4f4 100755 --- a/scripts/field_report.py +++ b/scripts/field_report.py @@ -60,6 +60,9 @@ class FieldInfo: # first byte. self.offset = node.get_field_offsetof() // 8 + def name(self) -> str: + return self.node.spelling + def record_use(self, _node: Cursor) -> None: self.lexical_uses += 1 @@ -178,20 +181,39 @@ class Ast: return self.fields[structure_type][field_name].record_use(node) - def sanity_checks(self) -> None: - """If the data looks wrong, raise a `SanityCheck` exception.""" + def sanity_check_failed(self, log: Optional[typing_util.Writable], + fmt: str, *args, **kwargs) -> None: + #pylint: disable=no-self-use,no-else-raise + msg = fmt.format(*args, **kwargs) + if log is None: + raise SanityCheck(msg) + else: + log.write('Warning: ' + msg + '\n') + + def sanity_checks(self, log: Optional[typing_util.Writable]) -> None: + """If the data looks wrong, signal it. + + If `log` is `None`, signaling means to raise an exception explaining + the first failure encountered. Otherwise signaling means calling + `log.write` with a message for each failure. + """ for type_name in sorted(self.fields): for field in self.fields[type_name].values(): if field.offset == -1: - raise SanityCheck('Could not determine offset of {}.{}. ' - .format(type_name, field)) + self.sanity_check_failed( + log, + 'Could not determine offset of {}.{}.', + type_name, field.name()) def run_analysis(self, files: List[str], - sanity_checks: bool = True) -> None: + log: Optional[typing_util.Writable] = None) -> None: + """Run analyses on the specified files. + + Pass `log` to `sanity_checks`. + """ self.read_field_definitions(files) self.read_field_usage(files) - if sanity_checks: - self.sanity_checks() + self.sanity_checks(log) def read_field_usage(self, filenames: List[str]) -> None: """Parse field usage in the given C source files.""" @@ -248,14 +270,18 @@ def main(): help="Omit the CSV header from the output") parser.add_argument('--no-sanity-checks', dest='sanity_checks', default=True, action='store_false', - help="Omit sanity checks, print output even if it's suspicious") + help="Bypass sanity checks, print output even if it's suspicious") parser.add_argument('--target', '-t', help="Target triple to build for (default: native build)") parser.add_argument('files', metavar='FILE', nargs='*', help="Source files to analyze") options = parser.parse_args() ast = Ast(options) - ast.run_analysis(options.files, sanity_checks=options.sanity_checks) + if options.sanity_checks: + sanity_log = None + else: + sanity_log = sys.stderr + ast.run_analysis(options.files, log=sanity_log) ast.report(options, sys.stdout) if __name__ == '__main__':