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

pextlib1.0: init err buffer, use err code if empty #186

Merged
merged 2 commits into from Jun 14, 2020

Conversation

neverpanic
Copy link
Member

curl < 7.60.0 does not initialize the error buffer, and cases have been reported where a curl download failed without the error buffer being set. This was the case on older systems that did not yet have curl 7.60.0, and caused random garbage to be printed.

0-initialize the buffer and check whether the buffer has a useful message before returning it. Generate a substitute message using curl_easy_strerror() from the returned error code if the error buffer is not set.

Closes: https://trac.macports.org/ticket/60581

Copy link
Contributor

@ryandesign ryandesign left a comment

Choose a reason for hiding this comment

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

Looks reasonable, just a typo in a comment ("doesn" -> "doesn't") and in a commit message ("parantheses" -> "parentheses"). My preference would be to move every file toward our customary 4 spaces for indentation, but that's a larger change and doesn't have to happen right now.

src/pextlib1.0/curl.c Outdated Show resolved Hide resolved
@ryandesign
Copy link
Contributor

Oh and the "tests: Fix curl isnewer test broken by braeburn update" commit shouldn't be in this PR. That's already been merged to master.

We indent this file with tabs and don't add spaces inside of
parentheses, so reformat the code that did not use this convention.
curl < 7.60.0 does not initialize the error buffer, and cases have been
reported where a curl download failed without the error buffer being
set. This was the case on older systems that did not yet have curl
7.60.0, and caused random garbage to be printed.

0-initialize the buffer and check whether the buffer has a useful
message before returning it. Generate a substitute message using
curl_easy_strerror() from the returned error code if the error buffer is
not set.

Closes: https://trac.macports.org/ticket/60581
@neverpanic
Copy link
Member Author

I pushed the "curl isnewer" commit to this branch as well to allow the tests to pass. It would have disappeared automatically when merging, but since I'm now pushing another version of the PR for your review comments anyway, I've rebased on top of master.

@ryandesign
Copy link
Contributor

It would have disappeared automatically when merging

Ah. I still have no idea how git works. 😐

Copy link
Member Author

@neverpanic neverpanic left a comment

Choose a reason for hiding this comment

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

Finally had a few minutes to test this, and it at least doesn't seem to break anything, although I cannot reproduce the original problem reported by Ryan.

I'll merge this now, since it doesn't break things.

@neverpanic neverpanic merged commit f535c26 into macports:master Jun 14, 2020
@neverpanic neverpanic deleted the ticket60581 branch June 14, 2020 11:51
@neverpanic
Copy link
Member Author

I couldn't reproduce and thus not verify that this does what I wrote it to do, but Travis just triggered such a case and it seems to work as expected: https://paste.macports.org/75dc136dfd83

Error: Failed to fetch py38-lxml: curl_multi_info_read() returned {.msg = CURLMSG_DONE, .data.result = 7 (!= CURLE_OK)}, but the error buffer is not set. curl_easy_strerror(.data.result): Couldn't connect to server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 participants