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

Comments

Projects
None yet
3 participants
@bnason-nf
Contributor

bnason-nf commented Oct 5, 2017

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

This comment has been minimized.

Contributor

bnason-nf commented Oct 5, 2017

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

bagder commented Oct 5, 2017

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

@bnason-nf

This comment has been minimized.

Contributor

bnason-nf commented Oct 5, 2017

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

Remove extraneous null check in curlx_strtoooft()
Closes curl#1950: curlx_strtoofft() doesn't fully protect against null 'str'
argument.

@bagder bagder closed this 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.