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_initinfo not called consistently #1103

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Nov 4, 2016

With the latest libcurl (v7.51.0) I ran into an odd behavior caused by Curl_initinfo not being called/copied over consistently. As a result, CURLINFO_FILETIME may oscillate between 0 and -1 unexpectedly.

  • If you run curl_easy_init, Curl_initinfo is not called. That means CURLINFO_FILETIME will be set to 0.
  • If you call curl_easy_reset on that handle, Curl_initinfo is called. This behavior is new starting in 22cfeac. That means CURLINFO_FILETIME will be set to -1.
  • If you call curl_easy_duphandle on that handle, Curl_initinfo is not called and the info is never set. That means CURLINFO_FILETIME will be set to 0.

This came up because of a test in HHVM (PHP interpreter) that tried to verify that two handles had the same info after calling curl_easy_duphandle. curl_init in HHVM would call curl_easy_init and curl_easy_reset internally, so CURLINFO_FILETIME would be set to -1. If you called curl_easy_duphandle the new handle would have CURLINFO_FILETIME set to 0.

It seems like for consistency the code could be doing a couple things to make the behavior consistent:

  1. Calling Curl_initinfo inside curl_easy_init
  2. Either calling Curl_initinfo in curl_easy_duphandle (if we don't care about not exactly duplicating the handle) or copying the data explicitly from one handle to the other.

@jay
Copy link
Member

jay commented Nov 3, 2016

initinfo is called on perform not init. I don't think there would be any ramifications to calling it in init as well. as far as duphandle goes I'm pretty sure it's supposed to be like that. duphandle copies the options not the info.

@jay
Copy link
Member

jay commented Nov 3, 2016

ah I just re-read what you wrote. you are saying since the info is not copied on duphandle it should initinfo. ok.

@FBNeal
Copy link
Author

FBNeal commented Nov 4, 2016

Yeah. The inconsistency I was seeing was because 22cfeac started calling initinfo on reset, so a handle which was reset would have different values than a handle which was freshly initialized or freshly duplicated. I only noticed because we were calling reset on new handles automatically (changed in facebook/hhvm@7f4b85b) but the inconsistency seemed worth raising here. :-)

- Call Curl_initinfo on init and duphandle.

Prior to this change the statistical and informational variables were
simply zeroed by calloc on easy init and duphandle. While zero is the
correct default value for almost all info variables, there is one where
it isn't (filetime initializes to -1).

Bug: curl#1103
Reported-by: Neal Poole
@mention-bot
Copy link

@jay, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @dfandrich and @captain-caveman2k to be potential reviewers.

@jay
Copy link
Member

jay commented Nov 4, 2016

Ok I turned this into a PR with what I think we should do, someone please review.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Ship it!

@bagder
Copy link
Member

bagder commented Nov 4, 2016

It'd be cool to add a test...

jay added a commit that referenced this pull request Nov 6, 2016
- Call Curl_initinfo on init and duphandle.

Prior to this change the statistical and informational variables were
simply zeroed by calloc on easy init and duphandle. While zero is the
correct default value for almost all info variables, there is one where
it isn't (filetime initializes to -1).

Bug: #1103
Reported-by: Neal Poole
@jay
Copy link
Member

jay commented Nov 6, 2016

It'd be cool to add a test...

Done, landed in 4564636.

@jay jay closed this Nov 6, 2016
@jay jay deleted the init-info-on-easy-init-and-duphandle branch November 6, 2016 03:14
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants