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

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

Closed
wants to merge 1 commit into from

Conversation

jay
Copy link
Member

@jay jay commented Nov 11, 2016

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

- 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
@mention-bot
Copy link

@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);
}
Copy link
Member

Choose a reason for hiding this comment

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

Possibly showing an error message here in case of failure?

Copy link
Member Author

@jay jay Nov 11, 2016

Choose a reason for hiding this comment

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

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");

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know

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.

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

@bagder
Copy link
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
Copy link
Member Author

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
Copy link
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 closed this in ee3c83f Dec 29, 2016
@jay jay deleted the fix_windows_remote-time branch December 29, 2016 03:36
mkhon pushed a commit to mkhon/curl that referenced this pull request Dec 31, 2016
- 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
@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.
Development

Successfully merging this pull request may close these issues.

None yet

3 participants