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

Backport 2.28: ssl_cache: misc improvements #8280

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Sep 29, 2023

This is a backport of the bug fix in #7298: functions in the ssl_cache module were returning 1 on many error conditions, unlike the rest of the library where errors are conveyed by an error code that's always negative.

There was significant refactoring in the ssl_cache module in 3.x, so cherry-picking the commits led to significant conflicts, and instead I mostly re-did the same changes:

  • Create a new error code — it had to have a different number in 2.28 because the one from 3.5 is taken in 2.28. Also, in 2.28, error.c needs to be updated.
  • Change functions to return a negative error code — I searched for return 1 or ret = 1 and changed to what seemed like sensible values.
  • Documentation update — I copied the text we used in 3.5.
  • In 2.28, only mbedtls_ssl_cache_get and mbedtls_ssl_cache_set are concerned, there's no mbedtls_ssl_cache_remove.
  • I included the changelog entry which was in a separate PR Changelog entry for 7298 #8278.

PR checklist

Please tick as appropriate and edit the reasons (e.g.: "backport: not needed because this is a new feature")

There was no good error to return in this case.

Signed-off-by: Gilles Peskine <[email protected]>
mbedtls_ssl_cache_get() and mbedtls_ssl_cache_set() returned 1 on many error
conditions. Change this to returning a negative MBEDTLS_ERR_xxx error code.

Completeness: after this commit, there are no longer any occurrences of
`return 1` or `ret = 1`.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm added bug needs-design-approval needs-review Every commit must be reviewed by at least two team members, component-tls needs-ci Needs to pass CI tests needs-reviewer This PR needs someone to pick it up for review size-xs Estimated task size: extra small (a few hours at most) labels Sep 29, 2023
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@minosgalanakis minosgalanakis added the priority-very-high Highest priority - prioritise this over other review work label Sep 29, 2023
@daverodgman daverodgman added this pull request to the merge queue Sep 29, 2023
@daverodgman daverodgman added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Sep 29, 2023
Merged via the queue into Mbed-TLS:mbedtls-2.28 with commit da635ab Sep 29, 2023
1 check passed
@minosgalanakis minosgalanakis mentioned this pull request Apr 11, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports bug component-tls needs-design-approval needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review priority-very-high Highest priority - prioritise this over other review work size-xs Estimated task size: extra small (a few hours at most)
Projects
Development

Successfully merging this pull request may close these issues.

3 participants