Skip to content

do not use string-to-integer conversions without error handling#4906

Merged
danmar merged 11 commits intodanmar:mainfrom
firewave:atoi-2
Apr 8, 2023
Merged

do not use string-to-integer conversions without error handling#4906
danmar merged 11 commits intodanmar:mainfrom
firewave:atoi-2

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

@firewave firewave commented Mar 21, 2023

I came across this in preparation of improving the error location of MathLib::to*(). I also wanted to improve the argument validation in CmdLineParser for a while now.

atoi() will return 0 if the conversion fails without any indication.

std::istringstream usage needs to be checked and it can overflow without error which can leading to wrong results. It also behaves differently with Visual Studio. See https://trac.cppcheck.net/ticket/10180 and https://trac.cppcheck.net/ticket/10181.

We also use(d) MathLib::to*Long() for non-literal conversions which required us to allow inputs which are not literals. I have not rolled back those changes yet since I still need to identify the changes. As I have a few more other tests cases to add I will do that in a different PR.

The enabled clang-tidy check warns about atoi() usage - see https://clang.llvm.org/extra/clang-tidy/checks/cert/err34-c.html.

@firewave firewave marked this pull request as draft March 21, 2023 12:54
@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Mar 21, 2023

The string we convert in getMinFormatStringOutputLength() in checkbufferoverrun.cpp might not contain any numbers so it is intentional that we might get 0. Looking at the documentation of the format specifiers it seems might also encounter some valid strings which need to be converted differently - but that's something completely different.

@firewave firewave force-pushed the atoi-2 branch 3 times, most recently from 508f7e8 to be35a05 Compare March 23, 2023 13:07
@firewave firewave marked this pull request as ready for review March 23, 2023 13:16
@firewave
Copy link
Copy Markdown
Collaborator Author

I filed https://trac.cppcheck.net/ticket/11631 about the Failed to instantiate template selfcheck warnings.

@firewave firewave marked this pull request as ready for review April 8, 2023 15:34
@danmar danmar merged commit f5e51ea into danmar:main Apr 8, 2023
@firewave firewave deleted the atoi-2 branch April 13, 2023 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants