tool_operate: Fix --remote-time incorrect times on Windows. draft1 #1121

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@jay
Member
jay commented Nov 11, 2016 edited

It was reported on the mailing list that --remote-time isn't working properly on Windows and the timestamp may be off.

In the FAQ it says:

During daylight savings time, when -R is used, curl will set a time that appears one hour off.


curl is using utime to do the modification. I tried these dates:

Last-Modified: Thu, 01 Jan 1970 00:00:30 GMT

curl filetime is 30 (correct), and it sets that using utime.
file's last modified FILETIME is 116444736300000000 (correct)

Last-Modified: Thu, 20 Aug 1970 11:33:50 GMT

curl filetime is 20000030 (correct) and it sets that using utime.
file's last modified FILETIME is 116644772300000000 (incorrect: Thu, 20 Aug 1970 12:33:50 GMT)
The FILETIME should actually be 116644736300000000.


Last-Modified is defined HTTP-date and must always be GMT/UTC. So I think we could set the FILETIMEs directly on Windows just by using SetFileTime instead.

My first take on this is attached, but it requires i64 as a literal suffix and 64-bit integers.


Edit: Uploaded a build of draft1 at the reporter's request:
curl_pr1121_draft1.zip

@jay jay tool_operate: Fix --remote-time incorrect times on Windows. draft1
- Use Windows API SetFileTime to set the file time instead of utime.

We can't use utime on Windows because it may apply a daylight saving
time offset to our UTC file time.

Bug: https://curl.haxx.se/mail/archive-2016-11/0033.html
Reported-by: Tim

Closes xxx
e2f71e4
@jay jay added the cmdline tool label Nov 11, 2016
@mention-bot

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

+ SetFileTime(hfile, NULL, (FILETIME *)&converted,
+ (FILETIME *)&converted);
+ CloseHandle(hfile);
+ }
@bagder
bagder Nov 11, 2016 Member

Possibly showing an error message here in case of failure?

@jay
jay Nov 11, 2016 edited Member

I notice we don't do that for utime so I didn't do it here. I'm not sure why it's not done. If it should be done for one should it be done for both? How about fprintf(config->global->errors, "Failed to set modification times");

@bagder
bagder Nov 11, 2016 Member

Right, it should be done for both. I guess it happens so rarely people haven't noticed or cared.

@jay
jay Nov 11, 2016 Member

Ok. Also is there a preferred way to handle when 64-bit is available and what literal suffix to use? I'm not sure what is going to be portable across all Windows compilers. I guess I could use curl_off_t and CURL_SUFFIX_CURL_OFF_T

@bagder
bagder Nov 11, 2016 Member

Yeah, those are the ones we have. Do we actually have non-64bit capable windows/compilers people still around?

@jay
jay Nov 11, 2016 Member

I don't know

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

Apart from comment that I leave you to consider, it looks fine!

@bagder
Member
bagder commented Nov 11, 2016

Oh, and maybe add a line to the FAQ about from which point in time this is fixed.

@jay
Member
jay commented Dec 27, 2016

I am thinking I will modify my code so the fix is only present when there's a >= 64-bit curl_off_t, and otherwise fallback to utime. The alternative is add functions that can do 64-bit arithmetic using only 32-bit integers. Thoughts?

@bagder
Member
bagder commented Dec 27, 2016

I am thinking I will modify my code so the fix is only present when there's a >= 64-bit curl_off_t, and otherwise fallback to utime

I think that's fine. It should be relatively rare with <64 bit curl_off_t on windows builds.

@jay jay added a commit that closed this pull request Dec 29, 2016
@jay jay tool_operate: Fix --remote-time incorrect times on Windows
- Use Windows API SetFileTime to set the file time instead of utime.

Avoid utime on Windows if possible because it may apply a daylight
saving time offset to our UTC file time.

Bug: https://curl.haxx.se/mail/archive-2016-11/0033.html
Reported-by: Tim

Closes #1121
ee3c83f
@jay jay closed this in ee3c83f Dec 29, 2016
@jay jay deleted the jay:fix_windows_remote-time branch Dec 29, 2016
@mkhon mkhon added a commit to mkhon/curl that referenced this pull request Dec 31, 2016
@jay @mkhon jay + mkhon tool_operate: Fix --remote-time incorrect times on Windows
- Use Windows API SetFileTime to set the file time instead of utime.

Avoid utime on Windows if possible because it may apply a daylight
saving time offset to our UTC file time.

Bug: https://curl.haxx.se/mail/archive-2016-11/0033.html
Reported-by: Tim

Closes curl#1121
e032df9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment