From 1eeca41472ef675db0a6a4bfbe09e9aaad055d2a Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 15 Oct 2018 12:01:35 +0100 Subject: [PATCH 01/40] 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 6b7899fe0..34555bc98 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 1b1f0a771..c617c08ba 100644 --- a/library/x509_crt.c +++ b/library/x509_crt.c @@ -1444,7 +1444,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, @@ -1471,7 +1471,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 5bce95865..8c851acfb 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -615,11 +615,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; @@ -776,15 +779,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 996033e3dfce2900eca72c606fb7f3821f0a82f5 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Mon, 15 Oct 2018 12:23:02 +0100 Subject: [PATCH 02/40] Adapt ChangeLog --- ChangeLog | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/ChangeLog b/ChangeLog index 606f8f0b2..68efe2e8d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -19,6 +19,10 @@ Bugfix in X.509 module. Fixes #2212. * Reduce stack usage of `mpi_write_hlp()` by eliminating recursion. Fixes #2190. + * 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 * Include configuration file in all header files that use configuration, From 818bac55ef59c2fdd5a3f5047977eb925aa567c4 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Fri, 26 Oct 2018 09:13:26 +0100 Subject: [PATCH 03/40] 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 26d02e132742ebc6bebe90abdc17575f2a16d635 Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 30 Oct 2018 09:29:25 +0000 Subject: [PATCH 04/40] 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++ ) From b0fc484188d1493f308677868cfae72e0b290b42 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Tue, 12 Mar 2019 10:48:18 -0400 Subject: [PATCH 05/40] Add crypto includes when generating errors in generate_errors.pl Adjusted generate_errors to have a configuration option of including crypto files. Turned on by default --- scripts/generate_errors.pl | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/scripts/generate_errors.pl b/scripts/generate_errors.pl index 0c1f7e16e..037f21495 100755 --- a/scripts/generate_errors.pl +++ b/scripts/generate_errors.pl @@ -3,23 +3,28 @@ # Generate error.c # # Usage: ./generate_errors.pl or scripts/generate_errors.pl without arguments, -# or generate_errors.pl include_dir data_dir error_file +# or generate_errors.pl include_dir data_dir error_file include_crypto use strict; -my ($include_dir, $data_dir, $error_file); +my ($include_dir, $data_dir, $error_file, $include_crypto); +my $crypto_dir = "crypto"; if( @ARGV ) { - die "Invalid number of arguments" if scalar @ARGV != 3; - ($include_dir, $data_dir, $error_file) = @ARGV; + die "Invalid number of arguments" if scalar @ARGV != 4; + ($include_dir, $data_dir, $error_file, $include_crypto) = @ARGV; -d $include_dir or die "No such directory: $include_dir\n"; -d $data_dir or die "No such directory: $data_dir\n"; + if( $include_crypto ) { + -d $crypto_dir or die "Crypto submodule not present\n"; + } } else { $include_dir = 'include/mbedtls'; $data_dir = 'scripts/data_files'; $error_file = 'library/error.c'; - + $include_crypto = 1; + -d $crypto_dir or die "Crypto submodule not present\n"; unless( -d $include_dir && -d $data_dir ) { chdir '..' or die; -d $include_dir && -d $data_dir @@ -48,6 +53,11 @@ close(FORMAT_FILE); $/ = $line_separator; my @files = <$include_dir/*.h>; + +if( $include_crypto ) { + @files = (<$include_dir/*.h>,<$crypto_dir/$include_dir/*.h>); +} + my @matches; foreach my $file (@files) { open(FILE, "$file"); @@ -73,8 +83,15 @@ foreach my $line (@matches) my ($error_name, $error_code) = $line =~ /(MBEDTLS_ERR_\w+)\s+\-(0x\w+)/; my ($description) = $line =~ /\/\*\*< (.*?)\.? \*\//; - die "Duplicated error code: $error_code ($error_name)\n" - if( $error_codes_seen{$error_code}++ ); + if( $error_codes_seen{$error_code}++ ) { + if( $include_crypto ) { + print "Duplicated error code: $error_code ($error_name)\n"; + next; + } + else { + die "Duplicated error code: $error_code ($error_name)\n" ; + } + } $description =~ s/\\/\\\\/g; if ($description eq "") { From 80d0419189af17557771477dbec1c8dc24b5ad04 Mon Sep 17 00:00:00 2001 From: Ron Eldor Date: Thu, 4 Apr 2019 15:02:01 +0300 Subject: [PATCH 06/40] 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 d4e945a86..628a53a22 100644 --- a/ChangeLog +++ b/ChangeLog @@ -83,6 +83,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 * Reduce RAM consumption during session renegotiation by not storing diff --git a/programs/ssl/ssl_server2.c b/programs/ssl/ssl_server2.c index 7858db305..45310e22b 100644 --- a/programs/ssl/ssl_server2.c +++ b/programs/ssl/ssl_server2.c @@ -282,8 +282,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 "" @@ -654,10 +660,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; @@ -676,7 +682,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; @@ -694,7 +703,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 || @@ -719,6 +730,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 ) @@ -729,6 +741,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 ef907604f846958c7e9d8e4527e91988e1eb8b2c Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 5 Apr 2019 03:33:18 -0400 Subject: [PATCH 07/40] Include crypto config when generating query config Adjusted generate_query_config.pl to have a configuration option of including the crypto config. Turned on by default. --- programs/ssl/query_config.c | 8 +++++ scripts/generate_query_config.pl | 55 ++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 16 deletions(-) diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index f2f7b46d6..41535ef4d 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -2586,6 +2586,14 @@ int query_config( const char *config ) } #endif /* MBEDTLS_PLATFORM_GMTIME_R_ALT */ +#if defined(MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER) + if( strcmp( "MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER", config ) == 0 ) + { + MACRO_EXPANSION_TO_STR( MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER ); + return( 0 ); + } +#endif /* MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER */ + /* If the symbol is not found, return an error */ return( 1 ); } diff --git a/scripts/generate_query_config.pl b/scripts/generate_query_config.pl index f15e03a35..cc49df4eb 100755 --- a/scripts/generate_query_config.pl +++ b/scripts/generate_query_config.pl @@ -18,7 +18,9 @@ use strict; +my $include_crypto = 1; my $config_file = "./include/mbedtls/config.h"; +my $crypto_config_file = "./crypto/include/mbedtls/config.h"; my $query_config_format_file = "./scripts/data_files/query_config.fmt"; my $query_config_file = "./programs/ssl/query_config.c"; @@ -33,31 +35,52 @@ MBEDTLS_PARAM_FAILED ); my $excluded_re = join '|', @excluded; -open(CONFIG_FILE, "$config_file") or die "Opening config file '$config_file': $!"; # This variable will contain the string to replace in the CHECK_CONFIG of the # format file my $config_check = ""; +my %defines_seen; +my @files = ($config_file); -while (my $line = ) { - if ($line =~ /^(\/\/)?\s*#\s*define\s+(MBEDTLS_\w+).*/) { - my $name = $2; - # Skip over the macro that prevents multiple inclusion - next if "MBEDTLS_CONFIG_H" eq $name; +if( @ARGV ) { + die "Invalid number of arguments" if scalar @ARGV != 1; + ($include_crypto) = @ARGV; +} - # Skip over the macro if it is in the ecluded list - next if $name =~ /$excluded_re/; +if( $include_crypto ) { + push(@files, $crypto_config_file); +} - $config_check .= "#if defined($name)\n"; - $config_check .= " if( strcmp( \"$name\", config ) == 0 )\n"; - $config_check .= " {\n"; - $config_check .= " MACRO_EXPANSION_TO_STR( $name );\n"; - $config_check .= " return( 0 );\n"; - $config_check .= " }\n"; - $config_check .= "#endif /* $name */\n"; - $config_check .= "\n"; +foreach my $file (@files) { + open(FILE, "$file") or die "Opening config file failed: '$file': $!"; + while (my $line = ) { + if ($line =~ /^(\/\/)?\s*#\s*define\s+(MBEDTLS_\w+).*/) { + my $name = $2; + + # Skip over the macro that prevents multiple inclusion + next if "MBEDTLS_CONFIG_H" eq $name; + + # Skip over the macro if it is in the excluded list + next if $name =~ /$excluded_re/; + + # Skip if this define is already added + if( $defines_seen{$name}++ ) { + print "Skipping $name, already added. \n"; + next; + } + + $config_check .= "#if defined($name)\n"; + $config_check .= " if( strcmp( \"$name\", config ) == 0 )\n"; + $config_check .= " {\n"; + $config_check .= " MACRO_EXPANSION_TO_STR( $name );\n"; + $config_check .= " return( 0 );\n"; + $config_check .= " }\n"; + $config_check .= "#endif /* $name */\n"; + $config_check .= "\n"; + } } + close(FILE); } # Read the full format file into a string From 79369cd8d91e6a8f3b4b0786743b96ae8b4f7c9b Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 5 Apr 2019 04:07:40 -0400 Subject: [PATCH 08/40] Add crypto includes when generating features in generate_features.pl Adjusted generate_features to have a configuration option of including crypto config. Turned on by default. --- library/version_features.c | 3 ++ scripts/generate_features.pl | 74 +++++++++++++++++++++++------------- 2 files changed, 51 insertions(+), 26 deletions(-) diff --git a/library/version_features.c b/library/version_features.c index 161788ca7..23b2a5a97 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -780,6 +780,9 @@ static const char *features[] = { #if defined(MBEDTLS_XTEA_C) "MBEDTLS_XTEA_C", #endif /* MBEDTLS_XTEA_C */ +#if defined(MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER) + "MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER", +#endif /* MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER */ #endif /* MBEDTLS_VERSION_FEATURES */ NULL }; diff --git a/scripts/generate_features.pl b/scripts/generate_features.pl index 1bd82ca2a..10aadb63e 100755 --- a/scripts/generate_features.pl +++ b/scripts/generate_features.pl @@ -3,18 +3,24 @@ use strict; -my ($include_dir, $data_dir, $feature_file); +my ($include_dir, $data_dir, $feature_file, $include_crypto); +my $crypto_include_dir = "crypto/include/mbedtls"; if( @ARGV ) { - die "Invalid number of arguments" if scalar @ARGV != 3; - ($include_dir, $data_dir, $feature_file) = @ARGV; + die "Invalid number of arguments" if scalar @ARGV != 4; + ($include_dir, $data_dir, $feature_file, $include_crypto) = @ARGV; -d $include_dir or die "No such directory: $include_dir\n"; -d $data_dir or die "No such directory: $data_dir\n"; + if( $include_crypto ) { + -d $crypto_include_dir or die "Crypto submodule not present\n"; + } } else { $include_dir = 'include/mbedtls'; $data_dir = 'scripts/data_files'; $feature_file = 'library/version_features.c'; + $include_crypto = 1; + -d $crypto_include_dir or die "Crypto submodule not present\n"; unless( -d $include_dir && -d $data_dir ) { chdir '..' or die; @@ -36,37 +42,53 @@ my $feature_format = ; close(FORMAT_FILE); $/ = $line_separator; +my %defines_seen; +my @files = ("$include_dir/config.h"); -open(CONFIG_H, "$include_dir/config.h") || die("Failure when opening config.h: $!"); +if( $include_crypto ) { + push(@files, "$crypto_include_dir/config.h"); +} my $feature_defines = ""; -my $in_section = 0; -while (my $line = ) -{ - next if ($in_section && $line !~ /#define/ && $line !~ /SECTION/); - next if (!$in_section && $line !~ /SECTION/); +foreach my $file (@files) { + open(FILE, "$file") or die "Opening config file failed: '$file': $!"; - if ($in_section) { - if ($line =~ /SECTION/) { - $in_section = 0; - next; + my $in_section = 0; + + while (my $line = ) + { + next if ($in_section && $line !~ /#define/ && $line !~ /SECTION/); + next if (!$in_section && $line !~ /SECTION/); + + if ($in_section) { + if ($line =~ /SECTION/) { + $in_section = 0; + next; + } + + my ($define) = $line =~ /#define (\w+)/; + + # Skip if this define is already added + if( $defines_seen{$define}++ ) { + print "Skipping $define, already added. \n"; + next; + } + + $feature_defines .= "#if defined(${define})\n"; + $feature_defines .= " \"${define}\",\n"; + $feature_defines .= "#endif /* ${define} */\n"; } - my ($define) = $line =~ /#define (\w+)/; - $feature_defines .= "#if defined(${define})\n"; - $feature_defines .= " \"${define}\",\n"; - $feature_defines .= "#endif /* ${define} */\n"; - } - - if (!$in_section) { - my ($section_name) = $line =~ /SECTION: ([\w ]+)/; - my $found_section = grep $_ eq $section_name, @sections; - - $in_section = 1 if ($found_section); - } -}; + if (!$in_section) { + my ($section_name) = $line =~ /SECTION: ([\w ]+)/; + my $found_section = grep $_ eq $section_name, @sections; + $in_section = 1 if ($found_section); + } + }; + close(FILE); +} $feature_format =~ s/FEATURE_DEFINES\n/$feature_defines/g; open(ERROR_FILE, ">$feature_file") or die "Opening destination file '$feature_file': $!"; From b4b1ae193b4a1d138fc6f06cf2b8d9f797b4773e Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 5 Apr 2019 04:16:12 -0400 Subject: [PATCH 09/40] Add description of generate_query_config.pl argument --- scripts/generate_query_config.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/generate_query_config.pl b/scripts/generate_query_config.pl index cc49df4eb..af3076be0 100755 --- a/scripts/generate_query_config.pl +++ b/scripts/generate_query_config.pl @@ -14,7 +14,7 @@ # information is used to automatically generate the body of the query_config() # function by using the template in scripts/data_files/query_config.fmt. # -# Usage: ./scripts/generate_query_config.pl without arguments +# Usage: ./scripts/generate_query_config.pl include_crypto use strict; From 92f91fc9ff1f446da154c465fc8ed54c454b5b22 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 5 Apr 2019 05:49:53 -0400 Subject: [PATCH 10/40] Add an option to use crypto source files in generated visual c project --- scripts/generate_visualc_files.pl | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/scripts/generate_visualc_files.pl b/scripts/generate_visualc_files.pl index 991397674..37e09a1f6 100755 --- a/scripts/generate_visualc_files.pl +++ b/scripts/generate_visualc_files.pl @@ -4,7 +4,7 @@ # 2010 # # Must be run from mbedTLS root or scripts directory. -# Takes no argument. +# Takes "include_crypto" as an argument, off by default. use warnings; use strict; @@ -18,10 +18,20 @@ my $vsx_main_file = "$vsx_dir/mbedTLS.$vsx_ext"; my $vsx_sln_tpl_file = "scripts/data_files/vs2010-sln-template.sln"; my $vsx_sln_file = "$vsx_dir/mbedTLS.sln"; +my $include_crypto = 0; +if( @ARGV ) { + die "Invalid number of arguments" if scalar @ARGV != 1; + ($include_crypto) = @ARGV; +} + my $programs_dir = 'programs'; my $header_dir = 'include/mbedtls'; my $source_dir = 'library'; +if( $include_crypto ) { + $source_dir = 'crypto/library'; +} + # Need windows line endings! my $vsx_hdr_tpl = <\r From 383d1fa6a531c4a1dfee91be2ee2a98f58db5bc0 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 4 Apr 2019 11:33:22 +0100 Subject: [PATCH 11/40] 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 91c6030584ab7a30ac4bd250cfca3ba3e250176a Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Tue, 9 Apr 2019 03:28:53 -0400 Subject: [PATCH 12/40] generate_errors.pl: add mbedtls header shadowing by crypto headers Abort script upon encountering a duplicated error --- scripts/generate_errors.pl | 41 +++++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 14 deletions(-) diff --git a/scripts/generate_errors.pl b/scripts/generate_errors.pl index 037f21495..6e04f9bf5 100755 --- a/scripts/generate_errors.pl +++ b/scripts/generate_errors.pl @@ -4,6 +4,7 @@ # # Usage: ./generate_errors.pl or scripts/generate_errors.pl without arguments, # or generate_errors.pl include_dir data_dir error_file include_crypto +# Include crypto can be either 0 (don't include) or 1 (include). On by default. use strict; @@ -16,15 +17,12 @@ if( @ARGV ) { -d $include_dir or die "No such directory: $include_dir\n"; -d $data_dir or die "No such directory: $data_dir\n"; - if( $include_crypto ) { - -d $crypto_dir or die "Crypto submodule not present\n"; - } } else { $include_dir = 'include/mbedtls'; $data_dir = 'scripts/data_files'; $error_file = 'library/error.c'; $include_crypto = 1; - -d $crypto_dir or die "Crypto submodule not present\n"; + unless( -d $include_dir && -d $data_dir ) { chdir '..' or die; -d $include_dir && -d $data_dir @@ -32,6 +30,10 @@ if( @ARGV ) { } } +if( $include_crypto ) { + -d $crypto_include_dir or die "Crypto submodule not present\n"; +} + my $error_format_file = $data_dir.'/error.fmt'; my @low_level_modules = qw( AES ARC4 ARIA ASN1 BASE64 BIGNUM BLOWFISH @@ -52,14 +54,31 @@ close(FORMAT_FILE); $/ = $line_separator; -my @files = <$include_dir/*.h>; +my %files; if( $include_crypto ) { - @files = (<$include_dir/*.h>,<$crypto_dir/$include_dir/*.h>); + my @crypto_headers = <$crypto_dir/$include_dir/*.h>; + my @mbedtls_files = <$include_dir/*.h>; + $files{$_}++ for (@crypto_headers); + + foreach my $file (@mbedtls_files) { + my $stripped_filename = substr($file, rindex($file,"/")+1, length($file)-rindex($file,"/")-1); + my $crypto_counterpart = "$crypto_dir/$include_dir/$stripped_filename"; + if ( exists $files{$crypto_counterpart} ){ + next; + } + else{ + push(@{$files{$file}}); + } + } +} +else{ + my @headers = <$include_dir/*.h>; + $files{$_}++ for (@headers); } my @matches; -foreach my $file (@files) { +foreach my $file (sort keys %files) { open(FILE, "$file"); my @grep_res = grep(/^\s*#define\s+MBEDTLS_ERR_\w+\s+\-0x[0-9A-Fa-f]+/, ); push(@matches, @grep_res); @@ -84,13 +103,7 @@ foreach my $line (@matches) my ($description) = $line =~ /\/\*\*< (.*?)\.? \*\//; if( $error_codes_seen{$error_code}++ ) { - if( $include_crypto ) { - print "Duplicated error code: $error_code ($error_name)\n"; - next; - } - else { - die "Duplicated error code: $error_code ($error_name)\n" ; - } + die "Duplicated error code: $error_code ($error_name)\n"; } $description =~ s/\\/\\\\/g; From da84e3215e057f4bdbc4468beaafeb2cda00387e Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 19 Feb 2019 16:59:33 +0000 Subject: [PATCH 13/40] 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 c2883a29bc836dae2b001471339f33a0d632fe7d Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 20 Feb 2019 15:01:56 +0000 Subject: [PATCH 14/40] 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 e62f9bbbf19e7a99e6c8ab6e0a760c5fc2964d30 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 21 Feb 2019 13:09:26 +0000 Subject: [PATCH 15/40] 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 3d3d552579d23d78e974cb7822d1d046fd4865fd Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 25 Feb 2019 17:01:55 +0000 Subject: [PATCH 16/40] 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 9f357d65d4c7ecd6c009471ba87cce19870e39e2 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Mon, 25 Feb 2019 11:35:05 +0000 Subject: [PATCH 17/40] 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 3e7a980d625050feea6da7ec41c1b6aec26ca586 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 27 Feb 2019 16:53:40 +0000 Subject: [PATCH 18/40] 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 4831145cdd143aaea64402e10207210a7b167ca1 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Wed, 27 Feb 2019 17:33:31 +0000 Subject: [PATCH 19/40] 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 ddf25a6095cf0dd021284db6064dd5f4e9d0a0c6 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 28 Feb 2019 11:52:39 +0000 Subject: [PATCH 20/40] 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 c5132ffc41632a34301621ae945b5d48d73b2af3 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 1 Mar 2019 09:54:44 +0000 Subject: [PATCH 21/40] 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 4cde8a05133f86d31b8155a54a10ddb7fbfaa32b Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 15:21:32 +0000 Subject: [PATCH 22/40] 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 e29ce70ca5761beb72e7a1c2d7d1f52927db2205 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 15:23:25 +0000 Subject: [PATCH 23/40] 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 7c1a73370bd6ffb091dbc7cb811ee447f6e176aa Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 16:25:38 +0000 Subject: [PATCH 24/40] 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 3a5f6c83bc25f7b96b8338319d0ca373934b7a81 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 5 Mar 2019 16:30:39 +0000 Subject: [PATCH 25/40] 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 1d95c539e981df548648f91b4818d5993e47f4b1 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 8 Mar 2019 11:12:19 +0000 Subject: [PATCH 26/40] 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 3c3da790d21c88645ca80800eb8e57532cf78011 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 8 Mar 2019 11:30:04 +0000 Subject: [PATCH 27/40] 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 fe9a67510e0f2ab97e7050c496ef77d97e57c0d8 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 4 Apr 2019 14:39:33 +0100 Subject: [PATCH 28/40] 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 8184df5de98200c88cefabaf395603020128dc27 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 5 Apr 2019 17:06:17 +0100 Subject: [PATCH 29/40] 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 0d1ca5110796e22ab22ebf7c7e23112c376d9940 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Tue, 9 Apr 2019 09:14:17 +0100 Subject: [PATCH 30/40] 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 492bc402a3d47012216e0fa5cf51564cb693c30e Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 11 Apr 2019 15:50:41 +0100 Subject: [PATCH 31/40] 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 9b11af42e2e6d8c18d2dbdb970c6a0933b1a81c5 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 12 Apr 2019 09:43:04 -0400 Subject: [PATCH 32/40] revert changes to generate_features.pl and generate_query_config.pl These script should depend solely on the external, mbedtls config --- library/version_features.c | 3 -- programs/ssl/query_config.c | 8 ---- scripts/generate_features.pl | 74 +++++++++++--------------------- scripts/generate_query_config.pl | 57 ++++++++---------------- 4 files changed, 43 insertions(+), 99 deletions(-) diff --git a/library/version_features.c b/library/version_features.c index 23b2a5a97..161788ca7 100644 --- a/library/version_features.c +++ b/library/version_features.c @@ -780,9 +780,6 @@ static const char *features[] = { #if defined(MBEDTLS_XTEA_C) "MBEDTLS_XTEA_C", #endif /* MBEDTLS_XTEA_C */ -#if defined(MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER) - "MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER", -#endif /* MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER */ #endif /* MBEDTLS_VERSION_FEATURES */ NULL }; diff --git a/programs/ssl/query_config.c b/programs/ssl/query_config.c index 41535ef4d..f2f7b46d6 100644 --- a/programs/ssl/query_config.c +++ b/programs/ssl/query_config.c @@ -2586,14 +2586,6 @@ int query_config( const char *config ) } #endif /* MBEDTLS_PLATFORM_GMTIME_R_ALT */ -#if defined(MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER) - if( strcmp( "MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER", config ) == 0 ) - { - MACRO_EXPANSION_TO_STR( MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER ); - return( 0 ); - } -#endif /* MBEDTLS_PSA_CRYPTO_KEY_FILE_ID_ENCODES_OWNER */ - /* If the symbol is not found, return an error */ return( 1 ); } diff --git a/scripts/generate_features.pl b/scripts/generate_features.pl index 10aadb63e..1bd82ca2a 100755 --- a/scripts/generate_features.pl +++ b/scripts/generate_features.pl @@ -3,24 +3,18 @@ use strict; -my ($include_dir, $data_dir, $feature_file, $include_crypto); -my $crypto_include_dir = "crypto/include/mbedtls"; +my ($include_dir, $data_dir, $feature_file); if( @ARGV ) { - die "Invalid number of arguments" if scalar @ARGV != 4; - ($include_dir, $data_dir, $feature_file, $include_crypto) = @ARGV; + die "Invalid number of arguments" if scalar @ARGV != 3; + ($include_dir, $data_dir, $feature_file) = @ARGV; -d $include_dir or die "No such directory: $include_dir\n"; -d $data_dir or die "No such directory: $data_dir\n"; - if( $include_crypto ) { - -d $crypto_include_dir or die "Crypto submodule not present\n"; - } } else { $include_dir = 'include/mbedtls'; $data_dir = 'scripts/data_files'; $feature_file = 'library/version_features.c'; - $include_crypto = 1; - -d $crypto_include_dir or die "Crypto submodule not present\n"; unless( -d $include_dir && -d $data_dir ) { chdir '..' or die; @@ -42,53 +36,37 @@ my $feature_format = ; close(FORMAT_FILE); $/ = $line_separator; -my %defines_seen; -my @files = ("$include_dir/config.h"); -if( $include_crypto ) { - push(@files, "$crypto_include_dir/config.h"); -} +open(CONFIG_H, "$include_dir/config.h") || die("Failure when opening config.h: $!"); my $feature_defines = ""; +my $in_section = 0; -foreach my $file (@files) { - open(FILE, "$file") or die "Opening config file failed: '$file': $!"; +while (my $line = ) +{ + next if ($in_section && $line !~ /#define/ && $line !~ /SECTION/); + next if (!$in_section && $line !~ /SECTION/); - my $in_section = 0; - - while (my $line = ) - { - next if ($in_section && $line !~ /#define/ && $line !~ /SECTION/); - next if (!$in_section && $line !~ /SECTION/); - - if ($in_section) { - if ($line =~ /SECTION/) { - $in_section = 0; - next; - } - - my ($define) = $line =~ /#define (\w+)/; - - # Skip if this define is already added - if( $defines_seen{$define}++ ) { - print "Skipping $define, already added. \n"; - next; - } - - $feature_defines .= "#if defined(${define})\n"; - $feature_defines .= " \"${define}\",\n"; - $feature_defines .= "#endif /* ${define} */\n"; + if ($in_section) { + if ($line =~ /SECTION/) { + $in_section = 0; + next; } - if (!$in_section) { - my ($section_name) = $line =~ /SECTION: ([\w ]+)/; - my $found_section = grep $_ eq $section_name, @sections; + my ($define) = $line =~ /#define (\w+)/; + $feature_defines .= "#if defined(${define})\n"; + $feature_defines .= " \"${define}\",\n"; + $feature_defines .= "#endif /* ${define} */\n"; + } + + if (!$in_section) { + my ($section_name) = $line =~ /SECTION: ([\w ]+)/; + my $found_section = grep $_ eq $section_name, @sections; + + $in_section = 1 if ($found_section); + } +}; - $in_section = 1 if ($found_section); - } - }; - close(FILE); -} $feature_format =~ s/FEATURE_DEFINES\n/$feature_defines/g; open(ERROR_FILE, ">$feature_file") or die "Opening destination file '$feature_file': $!"; diff --git a/scripts/generate_query_config.pl b/scripts/generate_query_config.pl index af3076be0..f15e03a35 100755 --- a/scripts/generate_query_config.pl +++ b/scripts/generate_query_config.pl @@ -14,13 +14,11 @@ # information is used to automatically generate the body of the query_config() # function by using the template in scripts/data_files/query_config.fmt. # -# Usage: ./scripts/generate_query_config.pl include_crypto +# Usage: ./scripts/generate_query_config.pl without arguments use strict; -my $include_crypto = 1; my $config_file = "./include/mbedtls/config.h"; -my $crypto_config_file = "./crypto/include/mbedtls/config.h"; my $query_config_format_file = "./scripts/data_files/query_config.fmt"; my $query_config_file = "./programs/ssl/query_config.c"; @@ -35,52 +33,31 @@ MBEDTLS_PARAM_FAILED ); my $excluded_re = join '|', @excluded; +open(CONFIG_FILE, "$config_file") or die "Opening config file '$config_file': $!"; # This variable will contain the string to replace in the CHECK_CONFIG of the # format file my $config_check = ""; -my %defines_seen; -my @files = ($config_file); +while (my $line = ) { + if ($line =~ /^(\/\/)?\s*#\s*define\s+(MBEDTLS_\w+).*/) { + my $name = $2; -if( @ARGV ) { - die "Invalid number of arguments" if scalar @ARGV != 1; - ($include_crypto) = @ARGV; -} + # Skip over the macro that prevents multiple inclusion + next if "MBEDTLS_CONFIG_H" eq $name; -if( $include_crypto ) { - push(@files, $crypto_config_file); -} + # Skip over the macro if it is in the ecluded list + next if $name =~ /$excluded_re/; -foreach my $file (@files) { - open(FILE, "$file") or die "Opening config file failed: '$file': $!"; - while (my $line = ) { - if ($line =~ /^(\/\/)?\s*#\s*define\s+(MBEDTLS_\w+).*/) { - my $name = $2; - - # Skip over the macro that prevents multiple inclusion - next if "MBEDTLS_CONFIG_H" eq $name; - - # Skip over the macro if it is in the excluded list - next if $name =~ /$excluded_re/; - - # Skip if this define is already added - if( $defines_seen{$name}++ ) { - print "Skipping $name, already added. \n"; - next; - } - - $config_check .= "#if defined($name)\n"; - $config_check .= " if( strcmp( \"$name\", config ) == 0 )\n"; - $config_check .= " {\n"; - $config_check .= " MACRO_EXPANSION_TO_STR( $name );\n"; - $config_check .= " return( 0 );\n"; - $config_check .= " }\n"; - $config_check .= "#endif /* $name */\n"; - $config_check .= "\n"; - } + $config_check .= "#if defined($name)\n"; + $config_check .= " if( strcmp( \"$name\", config ) == 0 )\n"; + $config_check .= " {\n"; + $config_check .= " MACRO_EXPANSION_TO_STR( $name );\n"; + $config_check .= " return( 0 );\n"; + $config_check .= " }\n"; + $config_check .= "#endif /* $name */\n"; + $config_check .= "\n"; } - close(FILE); } # Read the full format file into a string From e90205f9e6f9a8ed71c287c0afabc68c5db3e5a5 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 12 Apr 2019 09:49:30 -0400 Subject: [PATCH 33/40] generate_errors.pl: typo fix --- scripts/generate_errors.pl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/generate_errors.pl b/scripts/generate_errors.pl index 6e04f9bf5..ec550689f 100755 --- a/scripts/generate_errors.pl +++ b/scripts/generate_errors.pl @@ -4,7 +4,7 @@ # # Usage: ./generate_errors.pl or scripts/generate_errors.pl without arguments, # or generate_errors.pl include_dir data_dir error_file include_crypto -# Include crypto can be either 0 (don't include) or 1 (include). On by default. +# include_crypto can be either 0 (don't include) or 1 (include). On by default. use strict; @@ -31,7 +31,7 @@ if( @ARGV ) { } if( $include_crypto ) { - -d $crypto_include_dir or die "Crypto submodule not present\n"; + -d $crypto_dir or die "Crypto submodule not present\n"; } my $error_format_file = $data_dir.'/error.fmt'; From f67e3498631582d00e6ecf07c27281045ff926a7 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 15:17:02 +0100 Subject: [PATCH 34/40] 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 f025d5395ed3cdd76cac7e6ac0a27b9742783a27 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 15:18:02 +0100 Subject: [PATCH 35/40] 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 463f049ef0a2b17745b8b255ee997a879cb32797 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 12 Apr 2019 10:35:01 -0400 Subject: [PATCH 36/40] generate_errors.pl: refactor and simplify the code --- scripts/generate_errors.pl | 30 +++++++++--------------------- 1 file changed, 9 insertions(+), 21 deletions(-) diff --git a/scripts/generate_errors.pl b/scripts/generate_errors.pl index ec550689f..2fe202e8d 100755 --- a/scripts/generate_errors.pl +++ b/scripts/generate_errors.pl @@ -54,31 +54,19 @@ close(FORMAT_FILE); $/ = $line_separator; -my %files; - -if( $include_crypto ) { - my @crypto_headers = <$crypto_dir/$include_dir/*.h>; - my @mbedtls_files = <$include_dir/*.h>; - $files{$_}++ for (@crypto_headers); - - foreach my $file (@mbedtls_files) { - my $stripped_filename = substr($file, rindex($file,"/")+1, length($file)-rindex($file,"/")-1); - my $crypto_counterpart = "$crypto_dir/$include_dir/$stripped_filename"; - if ( exists $files{$crypto_counterpart} ){ - next; - } - else{ - push(@{$files{$file}}); - } +my @headers = (); +if ($include_crypto) { + @headers = <$crypto_dir/$include_dir/*.h>; + foreach my $header (<$include_dir/*.h>) { + my $basename = $header; $basename =~ s!.*/!!; + push @headers, $header unless -e "$crypto_dir/$include_dir/$basename"; } -} -else{ - my @headers = <$include_dir/*.h>; - $files{$_}++ for (@headers); +} else { + @headers = <$include_dir/*.h>; } my @matches; -foreach my $file (sort keys %files) { +foreach my $file (@headers) { open(FILE, "$file"); my @grep_res = grep(/^\s*#define\s+MBEDTLS_ERR_\w+\s+\-0x[0-9A-Fa-f]+/, ); push(@matches, @grep_res); From 021dc3f226daae466d0ee46552d20ebf64bc9022 Mon Sep 17 00:00:00 2001 From: Andrzej Kurek Date: Fri, 12 Apr 2019 10:51:27 -0400 Subject: [PATCH 37/40] generate_visualc_files.pl: add mbedtls source shadowing by crypto Running the generation script with "include_crypto" input parameter set to 1 makes the mbedtls sources being overshadowed by crypto sources. In case of any duplicate sources, crypto ones take precedence. --- scripts/generate_visualc_files.pl | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/scripts/generate_visualc_files.pl b/scripts/generate_visualc_files.pl index 37e09a1f6..e6545bc3a 100755 --- a/scripts/generate_visualc_files.pl +++ b/scripts/generate_visualc_files.pl @@ -4,7 +4,8 @@ # 2010 # # Must be run from mbedTLS root or scripts directory. -# Takes "include_crypto" as an argument, off by default. +# Takes "include_crypto" as an argument that can be either 0 (don't include) or +# 1 (include). Off by default. use warnings; use strict; @@ -27,10 +28,7 @@ if( @ARGV ) { my $programs_dir = 'programs'; my $header_dir = 'include/mbedtls'; my $source_dir = 'library'; - -if( $include_crypto ) { - $source_dir = 'crypto/library'; -} +my $crypto_dir = 'crypto'; # Need windows line endings! my $vsx_hdr_tpl = <; - my @sources = <$source_dir/*.c>; + + my @sources = (); + if ($include_crypto) { + @sources = <$crypto_dir/$source_dir/*.c>; + foreach my $file (<$source_dir/*.c>) { + my $basename = $file; $basename =~ s!.*/!!; + push @sources, $file unless -e "$crypto_dir/$source_dir/$basename"; + } + } else { + @sources = <$source_dir/*.c>; + } + map { s!/!\\!g } @headers; map { s!/!\\!g } @sources; From 117b8a45164bdb5ad2ab9e6809cbc923cffa338e Mon Sep 17 00:00:00 2001 From: Jaeden Amero Date: Wed, 17 Apr 2019 15:19:26 +0100 Subject: [PATCH 38/40] all.sh: Require i686-w64-mingw32-gcc version >= 6 Require mingw gcc version 6 or greater in order to ensure BCryptGenRandom() is available. --- tests/scripts/all.sh | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/scripts/all.sh b/tests/scripts/all.sh index 999fe6e4b..369df1591 100755 --- a/tests/scripts/all.sh +++ b/tests/scripts/all.sh @@ -1209,6 +1209,12 @@ component_build_mingw () { make CC=i686-w64-mingw32-gcc AR=i686-w64-mingw32-ar LD=i686-w64-minggw32-ld CFLAGS='-Werror -Wall -Wextra' WINDOWS_BUILD=1 SHARED=1 tests make WINDOWS_BUILD=1 clean } +support_build_mingw() { + case $(i686-w64-mingw32-gcc -dumpversion) in + [0-5]*) false;; + *) true;; + esac +} component_test_memsan () { msg "build: MSan (clang)" # ~ 1 min 20s From b2ee0b87826aa197da14e796f3305a81643f1b14 Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Fri, 12 Apr 2019 16:24:25 +0100 Subject: [PATCH 39/40] 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 1ae4886c828dce7643dc6d32d8f66f9100df5b2b Mon Sep 17 00:00:00 2001 From: Darryl Green Date: Thu, 18 Apr 2019 13:09:25 +0100 Subject: [PATCH 40/40] 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 ]