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

Add missing goto error #7825

Closed
wants to merge 1 commit into from
Closed

Add missing goto error #7825

wants to merge 1 commit into from

Conversation

qarmin
Copy link

@qarmin qarmin commented Oct 7, 2021

This PR adds missing goto error
Without this, failure was not handled. result was set to CURLE_OUT_OF_MEMORY, but in line 811 it was changed to somethings else, so it was useless

Looks that was copy paste bug from:

curl/lib/c-hyper.c

Lines 587 to 591 in e8c8775

if(hyper_request_set_uri(req, (uint8_t *)Curl_dyn_uptr(&r),
Curl_dyn_len(&r))) {
failf(data, "error setting path");
result = CURLE_OUT_OF_MEMORY;
}

@jay
Copy link
Member

jay commented Oct 7, 2021

This should be correct except that there is a bigger problem. There are a number of places in the hyper handling code where where goto error and result can be CURLE_OK. I think it should probably end like this

goto cleanup;
error:
if(result == CURLE_OK)
result = CURLE_OUT_OF_MEMORY;
cleanup:
....

@bagder

edit: on second thought, shouldn't the handshake and other things only be freed on error? does that code currently work?

@@ -805,7 +805,7 @@ static CURLcode CONNECT(struct Curl_easy *data,
if(hyper_request_set_uri(req, (uint8_t *)hostheader,
strlen(hostheader))) {
failf(data, "error setting path");
result = CURLE_OUT_OF_MEMORY;
goto error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should still assign result first before the goto, right? Otherwise there's no telling what the error is!

@bagder
Copy link
Member

bagder commented Oct 8, 2021

There are a number of places in the hyper handling code where where goto error and result can be CURLE_OK

Then those should be fixed to make sure they pass on the correct error code methinks!

@bagder
Copy link
Member

bagder commented Oct 8, 2021

shouldn't the handshake and other things only be freed on error? does that code currently work?

They run lots of tests fine in the CI and I've done repeated torture tests, even if they are a bit more limited when hyper is used they haven't triggered problems recently.

@jay
Copy link
Member

jay commented Oct 8, 2021

Then those should be fixed to make sure they pass on the correct error code methinks!

IMO goto error it would be good to have a default error there if an error code hasn't already been set. c-hyper Curl_http has this pattern as well, and sure they could all be changed from

if(foo)
  goto error;

to

if(foo) {
  result = CURLE_OUT_OF_MEMORY;
  goto error;
}

but I think it's better to put one check at the label so at least there is a default error, and also that way if a result is missed we don't end up returning CURLE_OK on error

bagder added a commit that referenced this pull request Oct 12, 2021
For every 'goto error', make sure the result variable holds the error
code for what went wrong.

Reported-by: Rafał Mikrut
Fixes #7825
@bagder
Copy link
Member

bagder commented Oct 12, 2021

I made a larger take at this problem in #7846

@bagder bagder closed this in 95c6abe Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants