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

Fix type conversion bug in ssl_context_info example program. #3567

Conversation

hanno-becker
Copy link

Previously, some code in programs/ssl/ssl_context_info.c converted the return value from fgetc() to char immediately, and then compared it to EOF. This doesn't work since EOF is a special value outside of the range of char (when embedded in int).

The PR fixes the code to store the result of fgetc() as an int, compare to EOF, and only on mismatch convert it to char for further processing.

Status

READY

Requires Backporting

NO

Hanno Becker added 2 commits August 14, 2020 10:52
Previously, the return value from `fgetc()` was immediately
converted to `char` and then compared to `EOF`. This doesn't
work since `EOF` is a special value outside of the range of
`char` (when embedded in `int`).

This commit fixes the code to store the result of `fgetc()`
as an `int`, compare to `EOF`, and only on mismatch convert
it to `char` for further processing.

Signed-off-by: Hanno Becker <hanno.becker@arm.com>
Signed-off-by: Hanno Becker <hanno.becker@arm.com>
@hanno-becker hanno-becker added bug mbed TLS team needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Aug 14, 2020
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

This was previously reported in #3449, originally with an incorrect fix but now with an updated fix and a non-regression test, which this PR is missing. I haven't reviewed either PR but I think #3449 is close to acceptable and we should take that one.

@hanno-becker
Copy link
Author

hanno-becker commented Aug 14, 2020

Yes, at the time of opening this PR, #3449 had a wider scope, which by now has been reduced to the correction of ssl_context_info. Feel free to handle the present PR as you see fit.

@gilles-peskine-arm
Copy link
Contributor

Thanks for the fix! @d3zd3z has written a non-regression test, so we'll merge this via #3799.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug needs-review Every commit must be reviewed by at least two team members,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants