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 invalid isspace call detected by PREfast #1925

Merged
merged 1 commit into from
May 12, 2020
Merged

Fix invalid isspace call detected by PREfast #1925

merged 1 commit into from
May 12, 2020

Conversation

BillyONeal
Copy link
Contributor

@BillyONeal BillyONeal commented May 12, 2020

D:\vcpkg\toolsrc\include\catch2\catch.hpp(11285): warning C6330: 'char' passed as _Param_(1) when 'unsigned char' is required in call to 'isspace'.
D:\vcpkg\toolsrc\include\catch2\catch.hpp(11288): warning C6330: 'char' passed as _Param_(1) when 'unsigned char' is required in call to 'isspace'.

ISO/IEC 9899:2011:
"7.4 Character handling <ctype.h>"/1
[...] In all cases the argument is an int, the value of which shall be
representable as an unsigned char or shall equal the value of the macro
EOF. If the argument has any other value, the behavior is undefined.

This means if isspace was passed a character like ñ it could corrupt
memory without the static_cast to treat it as a positive value after
integral promotion (and C libraries commonly use the int index supplied
as a key into a table which result in out of bounds access if the
resulting int is negative).

D:\vcpkg\toolsrc\include\catch2\catch.hpp(11285): warning C6330: 'char' passed as _Param_(1) when 'unsigned char' is required in call to 'isspace'.
D:\vcpkg\toolsrc\include\catch2\catch.hpp(11288): warning C6330: 'char' passed as _Param_(1) when 'unsigned char' is required in call to 'isspace'.

ISO/IEC 9899:2011:
"7.4 Character handling <ctype.h>"/1
[...] In all cases the argument is an int, the value of which shall be
representable as an unsigned char or shall equal the value of the macro
EOF. If the argument has any other value, the behavior is undefined.

This means if isspace was passed a character like ñ it could corrupt
memory without the static_cast to treat it as a positive value after
integral promotion (and C libraries commonly use the int index supplied
as a key into a table which result in out of bounds access if the
resulting int is negative).
BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request May 12, 2020
CMakeLists.txt: Turn on /analyze when development warnings are on.
catch.hpp: See submitted upstream: catchorg/Catch2#1925
downloads.cpp: PREfast can't see that check_exit is noreturn when f == nullptr.
files.cpp: PREfast detected this bogus both sides of the && testing the same thing.
strings.cpp: Same as the catch issue, isspace needs to be given unsigned.
system.process.cpp: suppress ownership transfer warning about process_info, and make ProcessInfo ensure handles are closed.
build.cpp: PREfast still doesn't understand check_exit
commands.integrate.cpp: More bad <ctype.h> calls.
@codecov
Copy link

codecov bot commented May 12, 2020

Codecov Report

Merging #1925 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1925   +/-   ##
=======================================
  Coverage   88.60%   88.60%           
=======================================
  Files         138      138           
  Lines        5606     5606           
=======================================
  Hits         4967     4967           
  Misses        639      639           

@horenmar horenmar added the Bug label May 12, 2020
@horenmar
Copy link
Member

uuuuuugh

@horenmar horenmar merged commit b1dcdc5 into catchorg:master May 12, 2020
BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request May 14, 2020
CMakeLists.txt: Turn on /analyze when development warnings are on.
catch.hpp: See submitted upstream: catchorg/Catch2#1925
downloads.cpp: PREfast can't see that check_exit is noreturn when f == nullptr.
files.cpp: PREfast detected this bogus both sides of the && testing the same thing.
strings.cpp: Same as the catch issue, isspace needs to be given unsigned.
system.process.cpp: suppress ownership transfer warning about process_info, and make ProcessInfo ensure handles are closed.
build.cpp: PREfast still doesn't understand check_exit
commands.integrate.cpp: More bad <ctype.h> calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants