Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use addition to wrap low-level error codes by high-level error codes #2462

Open
hanno-becker opened this issue Feb 22, 2019 · 3 comments
Labels
component-crypto Crypto primitives and low-level interfaces component-tls component-x509 enhancement good-first-issue Good for newcomers historical-reviewed Reviewed & agreed to keep legacy PR/issue

Comments

@hanno-becker
Copy link

Issue: If a high-level function (e.g. X.509) calls a low-level function (e.g. ASN.1) and the latter fails, the high-level function usually wraps the low-level error code by something like

ret = low_level();
if( ret != 0 )
   return( HIGH_LEVEL + ret );

Functionally, that's OK because low-level and high-level error codes don't have bits in common, but it has the following deficiencies:

  • It leads to larger code: If the high-level function is in turn called from somewhere else where do something like ret = high_level(); if( ret != 0 ) return( ret );, then the compiler cannot squash the two return statements into one because it cannot argue that HIGH_LEVEL + ret doesn't lead to 0 being returned.
  • It's fragile: If we ever make the mistake of returning a high-level error code from a low-level module, the addition + could in theory overflow and lead to unintended behaviour.
  • Static analyzers might notice the first point and complain.

Task: Switch to a different idiom, e.g. returning HIGH_LEVEL | ret instead of HIGH_LEVEL + ret, throughout the library.

@ciarmcom
Copy link

ARM Internal Ref: IOTSSL-2793

@irwir
Copy link
Contributor

irwir commented Apr 16, 2019

* It's fragile: If we ever make the mistake of returning a high-level error code from a low-level module, the addition `+` could in theory overflow and lead to unintended behaviour.

Still fragile in that sense.
Logical addition might not cure unintended behaviour; though it should prevent overfow and related undefined behaviour for arithmetic addition.

@yanesca yanesca added the good-first-issue Good for newcomers label Mar 17, 2020
@bensze01 bensze01 self-assigned this Apr 29, 2020
@gstrauss
Copy link
Contributor

gstrauss commented Feb 3, 2022

In development branch, it looks like this has been consolidated into include/mbedtls/error.h inline function mbedtls_error_add() which is used in macro MBEDTLS_ERROR_ADD(). If the names stay the same, changing to bitwise-or the results is a one line (one char) change:

--- a/include/mbedtls/error.h
+++ b/include/mbedtls/error.h
@@ -163,7 +163,7 @@ static inline int mbedtls_error_add( int high, int low,
     (void)file;
     (void)line;
 
-    return( high + low );
+    return( high | low );
 }
 
 /**

@bensze01 bensze01 removed their assignment Mar 30, 2022
@aditya-deshpande-arm aditya-deshpande-arm added historical-reviewing Currently reviewing (for legacy PR/issues) historical-reviewed Reviewed & agreed to keep legacy PR/issue and removed historical-reviewing Currently reviewing (for legacy PR/issues) labels Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component-crypto Crypto primitives and low-level interfaces component-tls component-x509 enhancement good-first-issue Good for newcomers historical-reviewed Reviewed & agreed to keep legacy PR/issue
Projects
None yet
Development

No branches or pull requests

8 participants