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.

Sign up for GitHub

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

Jump to bottom

programs/ssl/ssl_context_info.c fails to compile if char is unsigned #3794

Closed
d3zd3z opened this issue Oct 15, 2020 · 1 comment
Closed

programs/ssl/ssl_context_info.c fails to compile if char is unsigned #3794

d3zd3z opened this issue Oct 15, 2020 · 1 comment
Labels
bug good-first-issue Good for newcomers help-wanted This issue is not being actively worked on, but PRs welcome.

Comments

@d3zd3z
Copy link
Contributor

d3zd3z commented Oct 15, 2020

Description

  • Type: Bug
  • Priority: Minor

Bug

OS
linux

mbed TLS build:
Version: 2.24.0

Expected behavior

Actual behavior
When compiling on a target with char as unsigned (arm64 Linux), compilation fails with:

../programs/ssl/ssl_context_info.c: In function ‘read_next_b64_code’:                                │
../programs/ssl/ssl_context_info.c:382:16: error: comparison is always true due to limited range of d│
ata type [-Werror=type-limits]                                                                       │
  382 |     while( EOF != c )                                                                        │
      |                ^~                                                                            │
@gilles-peskine-arm
Copy link
Contributor

There are independent two pull requests to fix this, but both require changes and I don't think anybody is actively working on them: #3449 and #3567

@gilles-peskine-arm gilles-peskine-arm added bug help-wanted This issue is not being actively worked on, but PRs welcome. Product Backlog good-first-issue Good for newcomers labels Oct 15, 2020
@d3zd3z d3zd3z mentioned this issue Oct 16, 2020
Merged
4 tasks
d3zd3z added a commit to d3zd3z/mbedtls that referenced this issue Oct 16, 2020
@d3zd3z
Fix EOF issue in context_info test
737c3b9 
The context info test stores the result of `fgetc` in a 'char'.  On
platforms with signed characters, reading a 0xFF byte will result in
this character being -1, triggering an early end of file.

On platforms with an unsigned char, all characters will be valid, but
end of file won't be detected.

Fixes Mbed-TLS#3794.

Based on:
    commit 89cf0f3
    Author: Nayna Jain <[email protected]>
    Date:   Thu Aug 13 19:17:53 2020 +0000

        programs/ssl: Fixes incorrect EOF check in ssl_context_info.c

        read_next_b64_code() function, that parses base64 encoded input
        doesn't recognize the EOF and returns when "Too many bad symbols
        are detected". This issue got identified when gcc complained for
        type-limit error during cmake.

        This patch fixes the issue by changing the variable type to int
        and removing type-cast of fgetc() output to 'char'.

Signed-off-by: Nayna Jain <[email protected]>
Signed-off-by: David Brown <[email protected]>
d3zd3z pushed a commit to d3zd3z/mbedtls that referenced this issue Oct 16, 2020
@naynajain @d3zd3z
programs/ssl: Fixes incorrect EOF check in ssl_context_info.c
25b1f0a 
read_next_b64_code() function, that parses base64 encoded input
doesn't recognize the EOF and returns when "Too many bad symbols
are detected". This issue got identified when gcc complained for
type-limit error during cmake.

This patch fixes the issue by changing the variable type to int
and removing type-cast of fgetc() output to 'char'.

[[email protected]]
The context info test stores the result of `fgetc` in a 'char'.  On
platforms with signed characters, reading a 0xFF byte will result in
this character being -1, triggering an early end of file.

On platforms with an unsigned char, all characters will be valid, but
end of file won't be detected.

Fixes Mbed-TLS#3794.

Signed-off-by: Nayna Jain <[email protected]>
Signed-off-by: David Brown <[email protected]>
goodwaterwu pushed a commit to goodwaterwu/mbedtls that referenced this issue Jul 2, 2021
@naynajain
programs/ssl: Fix incorrect EOF check in ssl_context_info.c
0af8d43 
In `read_next_b64_code()`, the result of fgetc() is stored into a char,
but later compared against EOF, which is generally -1.  On platforms
where char is unsigned, this generates a compiler warning/error that the
comparison will never be true (causing a build failure).  The value will
never match, with the function ultimately bailing with a "Too many bad
symbols are detected" error.

On platforms with signed char, EOF is detected, but a file containing a
0xFF character will causes a premature end of file exit of the loop.

Fix this by changing the result to an int.

Fixes Mbed-TLS#3794.

Signed-off-by: Nayna Jain <[email protected]>
Signed-off-by: David Brown <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug good-first-issue Good for newcomers help-wanted This issue is not being actively worked on, but PRs welcome.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@d3zd3z @gilles-peskine-arm