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

curlx_strtoofft() doesn't fully protect against null "str" argument #1950

Closed
bnason-nf opened this issue Oct 5, 2017 · 5 comments
Closed

Comments

@bnason-nf
Copy link
Contributor

With curl 7.46.0, in curlx_strtoofft(), there is a check for a null "str" argument, but the remaining lines of the function don't guard against null. Currently, it it would cause a null pointer dereference if str were ever null.

So, either the first condition in this while loop is unnecessary if str can never be null:

while(str && *str && ISSPACE(*str))

Or the remaining code should be changed to handle a null str argument (maybe just an early out at the top of the function would be simplest).

@bnason-nf
Copy link
Contributor Author

By the way I'm happy to provide a patch and pull request if there is a preference for which way to go with this.

@bagder
Copy link
Member

bagder commented Oct 5, 2017

First, 7.46.0 is really old so we don't really care much how we did back then. But...

The code still has a similar construct. We just don't support a NULL pointer in the first argument so the check for that being non-NULL is completely superfluous. We should perhaps instead make than an assert.

@jay
Copy link
Member

jay commented Oct 5, 2017

I think he means 7.56, I can't find that code in 7.46. I agree it should be removed. It is the same as if you call strtoll or _strtoi64 with null for the first argument.

@bagder
Copy link
Member

bagder commented Oct 5, 2017

ah, makes sense. Yes please @bnason-nf, a PR would be nice!

@bnason-nf
Copy link
Contributor Author

Yes sorry, 7.46.0 is a typo, I meant the latest 7.56.0. I will put together a PR.

bnason-nf added a commit to bnason-nf/curl that referenced this issue Oct 5, 2017
Closes curl#1950: curlx_strtoofft() doesn't fully protect against null 'str'
argument.
@bagder bagder closed this as completed in 454dae0 Oct 6, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

3 participants