Hygiene/use mbedtls error strings in logs #1875

aggarw13 · 2020-04-03T23:41:35Z

Stringify mbed TLS error codes in logs (to improve debugging experience)

Description

Create two utilities, mbedtls_strerror_highlevel and mbedtls_strerror_lowlevel, to support stringification of mbed TLS (high-level and low-level) codes with constant strings. (Existing mbedtls_sterror involves population of buffer string at runtime which is memory intensive)
Switch all existing log sites of mbed TLS error code to use the new utilities

Checklist:

I have tested my changes. No regression in existing tests.
My code is Linted.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

…use-mbedtls-error-strings-in-logs

…-error-strings-in-logs

libraries/3rdparty/mbedtls/library/mbedtls_error.c

libraries/3rdparty/mbedtls/include/mbedtls/mbedtls_error.h

sarenameas · 2020-04-07T17:06:23Z

libraries/3rdparty/mbedtls/include/mbedtls/mbedtls_error.h

@@ -0,0 +1,58 @@
+/*


Also does this custom file belong in the third party folder?

Will move it to the 3rdparty/mbedtls_utlils folder once #1881 is merged

Have relocated files to mbedtls_utils

libraries/3rdparty/mbedtls/include/mbedtls/mbedtls_error.h

…m master

…-error-strings-in-logs

… include mbedtls_utils in Project files

lundinc2 · 2020-04-08T22:21:23Z

libraries/3rdparty/mbedtls_utils/mbedtls_error.c

+const char * mbedtls_strerror_highlevel( int errnum )
+{
+    const char * rc = NULL;
+    int use_ret;


Should initial this to 0. Notably the Espressif project will consider this as an error. (For the make project at least)

We discussed offline that this will follow the mbedtls coding style with the goal of eventually pushing it to mbedtls repository

lundinc2 · 2020-04-08T22:23:58Z

libraries/3rdparty/mbedtls_utils/mbedtls_error.c

+const char * mbedtls_strerror_lowlevel( int errnum )
+{
+    const char * rc = NULL;
+    int use_ret;


Same comment about initialization

Same as this reply

lundinc2 · 2020-04-08T22:24:12Z

libraries/3rdparty/mbedtls_utils/mbedtls_error.c

+
+    if( use_ret == 0 )
+    {
+        return NULL;


single point of return as per the coding standard

Same as this reply

Correct, this will be contributed to a third-party dependency so needs to be in that style.

lundinc2 · 2020-04-08T22:30:52Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

@@ -3472,7 +3476,9 @@ CK_DECLARE_FUNCTION( CK_RV, C_Sign )( CK_SESSION_HANDLE xSession,

                    if( lMbedTLSResult != CKR_OK )
                    {
-                        PKCS11_PRINT( ( "mbedTLS sign failed with error %d \r\n", lMbedTLSResult ) );
+                        PKCS11_PRINT( ( "mbedTLS sign failed with error %s : %s \r\n",
+                                        mbedtls_strerror_highlevel( lMbedTLSResult ),


It the function returns NULL this is technically UB, but most implementations will just print (NULL).

I did quick test on the Xtensa compiler and this hardfaults if NULL is returned.

Good point. To address this concern, we could return a default value like "<low-level-code-not-included>"

I'm thinking of adding file-specific constants and macros to log level-codes only if non-null.

static const char * pNoHighLevelMbedTlsCodeStr = "<No-High-Level-Code>"; static const char * pNoLowLevelMbedTlsCodeStr = "<No-Low-Level-Code>"; #define mbedTlsHighLevelCodeOrDefault( errnum ) \ { \ ( mbedtls_strerror_highlevel( errnum ) != NULL ) ? \ mbedtls_strerror_highlevel( errnum ) : pNoHighLevelMbedTlsCodeStr ); \ } #define mbedTlsLowLevelCodeOrDefault( errnum ) \ { \ ( mbedtls_strerror_lowlevel( errnum ) != NULL ) ? \ mbedtls_strerror_lowlevel( errnum ) : pNoLowLevelMbedTlsCodeStr ); \ }

lundinc2 · 2020-04-08T23:53:16Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

@@ -65,6 +65,35 @@
 #include <stdio.h>
 #include <string.h>

+/**
+ * @brief Represents string to be loggedwhen mbed TLS returned error


nit: logged when typo

I do not see it fixed. Also, I see the same at multiple places.

Also, please end comment with period.

Oops, I forgot to push the change. I will make the change in a follow-up PR.

gkwicker

110% awesome

gkwicker · 2020-04-09T18:10:50Z

libraries/3rdparty/mbedtls_utils/mbedtls_error.c

+
+    if( use_ret == 0 )
+    {
+        return NULL;


Correct, this will be contributed to a third-party dependency so needs to be in that style.

aggarg · 2020-04-09T01:23:03Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

@@ -65,6 +65,35 @@
 #include <stdio.h>
 #include <string.h>

+/**
+ * @brief Represents string to be loggedwhen mbed TLS returned error


I do not see it fixed. Also, I see the same at multiple places.

aggarg · 2020-04-09T01:23:35Z

libraries/abstractions/pkcs11/mbedtls/iot_pkcs11_mbedtls.c

@@ -65,6 +65,35 @@
 #include <stdio.h>
 #include <string.h>

+/**
+ * @brief Represents string to be loggedwhen mbed TLS returned error


Also, please end comment with period.

aggarg · 2020-04-09T19:16:35Z

libraries/3rdparty/mbedtls_utils/mbedtls_error.c

+        /* High level error codes */
+        /* */
+        /* BEGIN generated code */
+        #if defined( MBEDTLS_CIPHER_C )


Use of switch would result in a faster code, right? (The same was suggested by ARM here: Mbed-TLS/mbedtls#3176).

Will look into tackling this in a separate PR to avoid scope-creep.

aggarw13 added 7 commits March 16, 2020 22:42

Update iot_tls.c to log stringified error codes from mbedtls

828bd4e

Updated another file to log stringified mbedtls error codes

6e5a49b

Merge branch 'master' of github.com:aws/amazon-freertos into hygiene/…

b17f66a

…use-mbedtls-error-strings-in-logs

Break mbedtls stringifying utility by high and low level codes

958a018

Merge remote-tracking branch 'origin/master' into hygiene/use-mbedtls…

0c1cf41

…-error-strings-in-logs

Replace all mbedtls error logging sites with the new strerror utilities

1a2de08

Minor comment update

ac5fdd7

aggarw13 requested review from lundinc2 and gkwicker April 3, 2020 23:43

Update location of the new utility files

b5d188f

lundinc2 reviewed Apr 6, 2020

View reviewed changes

libraries/3rdparty/mbedtls/library/mbedtls_error.c Outdated Show resolved Hide resolved

aggarw13 added 6 commits April 6, 2020 15:45

Fix build error in utility source

873bb6e

Add utility source file in project files

e6ef81c

Fix remaining build issues with utility source

1557bb5

Update remaining Project files

5e0a898

Update utility header to Project files

cf9b865

Update Cypress makefile with utility source

14b80f9

sarenameas reviewed Apr 7, 2020

View reviewed changes

libraries/3rdparty/mbedtls/include/mbedtls/mbedtls_error.h Outdated Show resolved Hide resolved

sarenameas reviewed Apr 7, 2020

View reviewed changes

aggarw13 added 5 commits April 7, 2020 17:08

Fix header include in utility source

1c6b266

Run uncrustify formatting on Windows

45c9c67

Use uncrustified output from another developer

d1b0f21

Fix project file updates for Mediatek, Nuovoton and Microchip

725f109

Doc updates for utility files

f65ace7

aggarw13 requested a review from lundinc2 April 7, 2020 23:58

sarenameas reviewed Apr 8, 2020

View reviewed changes

libraries/3rdparty/mbedtls/include/mbedtls/mbedtls_error.h Outdated Show resolved Hide resolved

aggarw13 added 4 commits April 8, 2020 18:16

Relocate new utility files for merging directory structure change fro…

3637034

…m master

Merge remote-tracking branch 'origin/master' into hygiene/use-mbedtls…

f1842e7

…-error-strings-in-logs

Re-locate mbedtls utilities files to mbedtls_utils folder; attempt to…

fa0f698

… include mbedtls_utils in Project files

More project file updates for relocation

308ed4e

aggarw13 added 4 commits April 8, 2020 20:02

Minor fix in mbedtls_utils path

d3fd613

More project updates

d43636c

Add mbedtls_utils include path in 3rdparty::mbedtls CMake target

858b585

Add mbedtls_utils include path in remaining Project Makefiles

e5956c2

lundinc2 reviewed Apr 8, 2020

View reviewed changes

aggarw13 added 2 commits April 8, 2020 23:49

Fix typo in header doc

3caa011

Address issue of NULL logging from new utilities

921a1a6

aggarw13 requested a review from lundinc2 April 8, 2020 23:52

lundinc2 reviewed Apr 8, 2020

View reviewed changes

lundinc2 previously approved these changes Apr 8, 2020

View reviewed changes

Fix typos in macro names and doc

458f449

aggarw13 dismissed lundinc2’s stale review via 458f449 April 9, 2020 00:10

aggarw13 requested a review from lundinc2 April 9, 2020 00:13

Fix build errors

980f70d

lundinc2 approved these changes Apr 9, 2020

View reviewed changes

gkwicker approved these changes Apr 9, 2020

View reviewed changes

aggarg reviewed Apr 9, 2020

View reviewed changes

aggarw13 merged commit 9db26b8 into master Apr 9, 2020

aggarw13 deleted the hygiene/use-mbedtls-error-strings-in-logs branch April 9, 2020 19:27

aggarw13 mentioned this pull request Apr 9, 2020

Fix doc typos #1891

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hygiene/use mbedtls error strings in logs #1875

Hygiene/use mbedtls error strings in logs #1875

aggarw13 commented Apr 3, 2020 •

edited

Loading

sarenameas Apr 7, 2020

aggarw13 Apr 7, 2020

aggarw13 Apr 8, 2020

lundinc2 Apr 8, 2020

aggarw13 Apr 8, 2020

lundinc2 Apr 8, 2020

aggarw13 Apr 8, 2020

lundinc2 Apr 8, 2020

aggarw13 Apr 8, 2020

gkwicker Apr 9, 2020

lundinc2 Apr 8, 2020

aggarw13 Apr 8, 2020 •

edited

Loading

aggarw13 Apr 8, 2020

lundinc2 Apr 8, 2020

aggarw13 Apr 9, 2020

aggarg Apr 9, 2020

aggarg Apr 9, 2020

aggarw13 Apr 9, 2020

gkwicker left a comment

gkwicker Apr 9, 2020

aggarg Apr 9, 2020

aggarg Apr 9, 2020

aggarg Apr 9, 2020

aggarw13 Apr 9, 2020

Hygiene/use mbedtls error strings in logs #1875

Hygiene/use mbedtls error strings in logs #1875

Conversation

aggarw13 commented Apr 3, 2020 • edited Loading

Stringify mbed TLS error codes in logs (to improve debugging experience)

Description

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aggarw13 Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkwicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aggarw13 commented Apr 3, 2020 •

edited

Loading

aggarw13 Apr 8, 2020 •

edited

Loading