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

Jump to bottom

gopher: NULL pointer dereference #3617

Closed
bagder opened this issue Feb 25, 2019 · 2 comments
Closed

gopher: NULL pointer dereference #3617

bagder opened this issue Feb 25, 2019 · 2 comments
Labels
GOPHER

Comments

@bagder
Copy link
Member

bagder commented Feb 25, 2019

I did this

Coverity pointed out this line (CID 1442746), as a possibly doing a NULL pointer dereference:

curl/lib/gopher.c

Line 96 in 509a0d9

gopherpath = strdup(path);

It seems possible to happen if you build a URL from parts and pass to curl with CURLOPT_CURLU.

(@danielgustafsson we talked on IRC about you grabbing this, which is why you are assigned. I submitted it here just to make sure we don't forget. Feel free to unassign if you're busy.)

I expected the following

It shouldn't be necesarry to check path for NULL there. We should either bail out earlier if it is, or we should make sure that it is never NULL.

curl/libcurl version

current master (509a0d9)

operating system

All
@bagder bagder added the GOPHER label Feb 25, 2019
@danielgustafsson
Copy link
Member

Thanks for adding it, I am indeed looking at this but my current travel schedule slowed things down a bit.

@bagder
Copy link
Member Author

bagder commented Mar 4, 2019

I wrote up a new test to see if this path == NULL was actually possible to trigger, and when doing that it was obvious: no. We can't actually end up with path set to NULL in the gopher code, so the simple fix here is just to remove the check for that.

When a URL is passed with CURLOPT_URL it can't happen because then we always set a default path. The idea was then that possibly when set with CURLOPT_CURLU it would be possible. But no, due to this logic:

curl/lib/transfer.c

Lines 1494 to 1502 in 2591a49

if(!data->change.url && data->set.uh) {
CURLUcode uc;
uc = curl_url_get(data->set.uh,
CURLUPART_URL, &data->set.str[STRING_SET_URL], 0);
if(uc) {
failf(data, "No URL set!");
return CURLE_URL_MALFORMAT;
}
}

As this generates a full URL that then later is parsed to pieces again and then we get the default path if it wasn't set previously.

The new test (test 659) that verifies that this is true is part of my PR #3641 - I instead found another CURLU bug that I ended up fixing there.

bagder added a commit that referenced this issue Mar 4, 2019
@bagder
gopher: remove check for path == NULL
646c6eb 
Since it can't be NULL and it makes Coverity believe we lack proper NULL
checks. Verified by test 659 coming separately in PR #3641.

Pointed out by Coverity CID 1442746.

Fixes #3617
@bagder bagder mentioned this issue Mar 4, 2019
Closed
@bagder bagder closed this as completed in 39c2962 Mar 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
GOPHER
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

gopher: remove check for path == NULL curl/curl
2 participants
@bagder @danielgustafsson