Curl_initinfo not called consistently #1103

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@FBNeal
FBNeal 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
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
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
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. :-)

@jay jay easy: Initialize info variables on easy init and duphandle
- 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
0341236
@mention-bot

@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
Member
jay commented Nov 4, 2016

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

@bagder
bagder approved these changes Nov 4, 2016 View changes

Ship it!

@bagder
Member
bagder commented Nov 4, 2016

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

@jay jay added a commit that referenced this pull request Nov 6, 2016
@jay jay easy: Initialize info variables on easy init and duphandle
- 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
4564636
@jay
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 jay:init-info-on-easy-init-and-duphandle branch Nov 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment