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

Improper use of GetTickCount #3437

Closed
mkujawa opened this issue Jan 4, 2019 · 14 comments

Comments

@mkujawa
Copy link

commented Jan 4, 2019

e9ababd#diff-0a54b726bba1300aed45ad5693c349d1 has introduced a warning regression by switching from a compile-time check to a run-time check, but highlights a real bug.

I did this

Compiled for windows with msvc /analyze and many warnings enabled

I expected the following

Clean compilation. But instead,

  curl\lib\timeval.c(55): error C2220: warning treated as error - no 'object' file generated
  curl\lib\timeval.c(50) : warning C28159: Consider using 'GetTickCount64' instead of 'GetTickCount'. Reason: GetTickCount overflows roughly every 49 days.  Code that does not take that into account can loop indefinitely.  GetTickCount64 operates on 64 bit values and does not have that problem

And that warning is highlighting a real bug with this code. GetTickCount() should not be used as a monotonically increasing time source. Instead, you should store your own count and increment it by the change in GetTickCount().

Pseudocode:

  static DWORDLONG ticks = {};
  static DWORD lastMilliseconds = 0;
  DWORD curMilliseconds = GetTickCount();
  if ( lastMilliseconds )
  {
    ticks += (curMilliseconds - lastMilliseconds);
  }
  lastMilliseconds = curMilliseconds;
  now.tv_sec = (time_t)(ticks / 1000);
  now.tv_usec = (unsigned int)(ticks % 1000);

This will have no overflow issues so long as it's called more frequently than once every 2^31-1 milliseconds. My pseudocode does have issues with multiple threads, though.

curl/libcurl version

251cabf

operating system

Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x64

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

Won't that cause multi-threading issues? (which I then noticed you also mentioned)

@bagder bagder added the Windows label Jan 4, 2019

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

A proper fix would rather instead use GetTickCount64() if a new enough Windows version is used.

@mkujawa

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

While GetTickCount64 is an easy and proper fix for newer windows, it would still leave older windows vulnerable to overflow. If there continues to be a GetTickCount fallback, it should be correct.

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

I won't disagree with that. But I can note that not a single person has reported an actual problem with this in the past so I don't consider it a very big problem...

@jay

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

We recently accepted an enhancement on this function to use QueryPerformance instead of GetTickCount64 for Vista+ for higher resolution. The issue reported here AFAICT affects <=XP which has been a known issue (or, at least, I knew about it...).

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

Ah right then GetTickCount64() isn't going to be useful since when that is available, a better solution is already used.

The amount of users on <= XP that cares about this (minor) issue I would suspect is rather small. I'll nominate it for KNOWN_BUGS and be done with it.

@mkujawa

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

That still leaves the compiler warning

@bagder

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

We will happily appreciate and welcome patches fixing that.

@jay

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

The warning is techinically correct. The analyzer is not smart enough to understand that the function is only called on versions of Windows that don't have GetTickCount64. You may be able to __pragma push and pop disabling the warning for just that area.

@MarcelRaad

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

Or maybe the else branch using GetTickCount could just be disabled at compile time when targeting >= Vista? The VS code analysis that complains about this only runs with the Vista+ toolset anyway, and it's just dead code in that case.

@mkujawa

This comment has been minimized.

Copy link
Author

commented Jan 22, 2019

Or maybe the else branch using GetTickCount could just be disabled at compile time when targeting >= Vista? The VS code analysis that complains about this only runs with the Vista+ toolset anyway, and it's just dead code in that case.

That was the old approach. The current code supports compiling a single binary that will use QueryPerformanceCounter on Vista and GetTickCount on <= XP. The windows api level define could be checked, but that still wouldn't fix the case of compiling with the new tools but targeting WINVER 0x0500

@MarcelRaad

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Code analysis is only supported when compiling with the non-XP toolset, so the minimum WINVER/_WIN32_WINNT is 0x0600 in this case.

The old approach was supporting only GetTickCount when targeting pre-Vista and only GetTickCount64 when targeting Vista or later, which was then changed to supporting both GetTickCount and QueryPerformanceCounter in both cases. What I mean is supporting both when targeting pre-Vista and only QueryPerformanceCounter when targeting Vista or later, in which case GetTickCount can never be called anyway.

@mkujawa

This comment has been minimized.

Copy link
Author

commented Jan 23, 2019

cl /D_WIN32_WINNT=0x0500 /D_WINVER=0x0500 /analyze a.cpp
Microsoft (R) C/C++ Optimizing Compiler Version 19.00.24215.1 for x86
Copyright (C) Microsoft Corporation.  All rights reserved.

a.cpp
c:\scratch\a.cpp(5) : warning C28159: Consider using 'GetTickCount64' instead of 'GetTickCount'. Reason: GetTickCount overflows roughly every 49 days.  Code that does not take that into account can loop indefinitely.  GetTickCount64 operates on 64 bit values and does not have that problem

Just because you're using the new compiler and new windows headers doesn't mean you're defining _WIN32_WINNT to be >= 0x0600. You can still target <= XP with the new tools. (It would be nice if the MS headers only added that annotation if GetTickCount64 was available...)

That said, it may be worth eliding the run-time check if pre-vista support isn't being specified by _WIN32_WINNT. That doesn't invalidate the warning disable I'm adding, as it would still be possible to compile that code with the newer platform headers.

@MarcelRaad

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Ah, wow! Last time I tried (probably with 0x0501 and in the VS 2012 era), I got hard compile errors when even including <windows.h>. At least it's unsupported [0] and the resulting application won't even run on the target Windows version when using ATL or not being extremely careful which APIs to use as some lost their target version conditions (which is why the XP toolsets use the Windows 7.1 SDK).

[0] https://visualstudiomagazine.com/articles/2012/10/10/vs2012-update-preview-available.aspx

@jay jay closed this in b0a43aa Jan 28, 2019

@lock lock bot locked as resolved and limited conversation to collaborators Apr 28, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.