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

curl_error: return an empty string if no error occurred #3901

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Contributor

@jay jay commented Mar 1, 2019

CURLOPT_ERRORBUFFER doc says "Do not rely on the contents of the
buffer unless an error code was returned." 1

Prior to this change the error buffer was returned even if no error had
occurred, and that buffer may contain incorrect information in such a
case. 2

Closes #xxxx


Also I noticed a comment "CURLE_PARTIAL_FILE is returned by HEAD requests". I don't see that happening unless HEAD was sent as a CURLOPT_CUSTOMREQUEST... The right way is CURLOPT_NOBODY, also see https://daniel.haxx.se/blog/2015/09/11/unnecessary-use-of-curl-x/

php-src/ext/curl/interface.c

Lines 3114 to 3120 in 54ef8d1

error = curl_easy_perform(ch->cp);
SAVE_CURL_ERROR(ch, error);
/* CURLE_PARTIAL_FILE is returned by HEAD requests */
if (error != CURLE_OK && error != CURLE_PARTIAL_FILE) {
smart_str_free(&ch->handlers->write->buf);
RETURN_FALSE;
}

/cc @bagder @Schnitzel

CURLOPT_ERRORBUFFER doc says "Do not rely on the contents of the
buffer unless an error code was returned." [1]

Prior to this change the error buffer was returned even if no error had
occurred, and that buffer may contain incorrect information in such a
case. [2]

[1]: https://curl.haxx.se/libcurl/c/CURLOPT_ERRORBUFFER.html
[2]: curl/curl#3629

Closes #xxxx
@bagder
Copy link
Contributor

bagder commented Mar 1, 2019

Looks like a good fix.

That CURLE_PARTIAL_FILE thing looks like a separate bug. curl does not return that error for HEAD requests, only if you trick it with weirdo option combinations and then the user asked for it.

ch->err.str[CURL_ERROR_SIZE] = 0;
RETURN_STRING(ch->err.str);
} else {
RETURN_STRING("");
Copy link
Member

Choose a reason for hiding this comment

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

This should use RETURN_EMPTY_STRING().

@nikic nikic added the Bug label Mar 1, 2019
@nikic
Copy link
Member

nikic commented Mar 1, 2019

Merged as 5025eb0 into 7.2+ together with the suggested tweak. Thanks!

Would you like to also submit a PR to drop the CURLE_PARTIAL_FILE check? At least the very least it should be fine for master, even if it breaks some code using incorrect options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants