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

Jump to bottom

Division by zero in Curl_now #3572

Closed
pgroke-dt opened this issue Feb 14, 2019 · 13 comments
Closed

Division by zero in Curl_now #3572

pgroke-dt opened this issue Feb 14, 2019 · 13 comments
Labels
crash Windows Windows-specific

Comments

@pgroke-dt
Copy link
Contributor

I did this

Execute unit tests for our application, which uses libcurl. Sometimes we see a crash in Curl_now with STATUS_INTEGER_OVERFLOW at the count.QuadPart / freq.QuadPart division. It's not every time, but with two of our tests we see one crash every 10 ~ 100 runs. So not really that rare.

I expected the following

No crash.

libcurl version

#define LIBCURL_VERSION "7.64.0"
#define LIBCURL_TIMESTAMP "2019-02-06"

Operating system

Windows >= Vista

What I think happens

This is probably a data race in Curl_now. When two or more threads call Curl_now at the same time, and it's never been called before, the "first" thread will enter the if(isVistaOrGreater == -1) block. That first thread will set isVistaOrGreater = 1 before setting the static freq variable. (Strictly the order is undefined, because no synchronization is used. But because QueryPerformanceCounter is an external function, the compiler will normally generate code that sets isVistaOrGreater = 1 before calling QueryPerformanceCounter to initialize freq. Even though freq is a function local static).

So the window of vulnerability is from when the first thread sets isVistaOrGreater = 1 to when QueryPerformanceCounter actually writes the result into the freq variable. Not particularly long, but this can easily be a few dozen or hundred cycles (and of course much much longer if there's a context switch).

If a second thread executes Curl_now during that time window, it will see isVistaOrGreater = 1, skip the initialization and go on to divide by freq -- which is still zero at that time. The integer division by zero then causes a CPU exception/trap/whatever which is handled by the OS and converted into a structured exception (SEH).

Side note: When dividing by zero we would normally expect STATUS_INTEGER_DIVIDE_BY_ZERO. The reason we see STATUS_INTEGER_OVERFLOW is probably that very shortly after the division by zero, the memory location for freq (which is the divisor) is set to a non-zero value. So when the "integer division exception" handler of the Windows kernel later reads the divisor, it will find it non-zero, and therefor think there was an integer overflow. See https://blogs.msdn.microsoft.com/oldnewthing/20141002-00/?p=43933/ for some more information about the topic.

Calling Curl_now during curl_global_init would probably fix the issue.
@bagder bagder added crash Windows Windows-specific labels Feb 14, 2019
@bagder
Copy link
Member

bagder commented Feb 14, 2019

Right, the use of statics like that is not a good idea. We need something better... /cc @pps83

@bagder
Copy link
Member

bagder commented Feb 14, 2019

I propose we just make it a build-time decision: if someone wants to build libcurl for Windows pre-vista, they need to set some define or something and then we go with the old code and for the rest we go with the modern take.

@pgroke-dt
Copy link
Contributor Author

You could also use thread safe function local static init. Something like:

static LARGE_INTEGER qpc_freq;

static int init_qpc() {
  if(Curl_verify_windows_version(6, 0, PLATFORM_WINNT,
                                 VERSION_GREATER_THAN_EQUAL)) {
    return QueryPerformanceFrequency(&qpc_freq) != 0;
  } else {
    return 0;
  }
}

struct curltime Curl_now(void)
{
  struct curltime now;
  static int const useQpc = init_qpc(); // thread safe 
  if(useQpc) { /* QPC timer might have issues pre-Vista */
    LARGE_INTEGER count;
    QueryPerformanceCounter(&count);
    now.tv_sec = (time_t)(count.QuadPart / qpc_freq.QuadPart);
    now.tv_usec =
      (int)((count.QuadPart % qpc_freq.QuadPart) * 1000000 / qpc_freq.QuadPart);
  }
  else {
...

@MarcelRaad
Copy link
Member

if someone wants to build libcurl for Windows pre-vista, they need to set some define or something

That define already exists: _WIN32_WINNT being < 0x600 :-)

@bagder
Copy link
Member

bagder commented Feb 14, 2019

Another take could be to store the dynamically detected windows version in win32_init(), called from curl_global_init() since that is only done once and already documented to not be thread-safe.

@pgroke-dt
Copy link
Contributor Author

Not only the Windows version but also the QPC frequency ;)

@bagder
Copy link
Member

bagder commented Feb 14, 2019 •
edited

Written blindly, but perhaps just something like this:

struct curltime Curl_now(void)
{
  struct curltime now;
#if !defined(_WIN32_WINNT) || !defined(_WIN32_WINNT_VISTA) ||   \
  (_WIN32_WINNT < _WIN32_WINNT_VISTA)
  /* for really old Windows versions */
  DWORD milliseconds = GetTickCount();
  now.tv_sec = milliseconds / 1000;
  now.tv_usec = (milliseconds % 1000) * 1000;
#else
  /* Vista or later */
  {
    LARGE_INTEGER count;
    QueryPerformanceCounter(&count);
    now.tv_sec = (time_t)(count.QuadPart / freq.QuadPart);
    now.tv_usec =
      (int)((count.QuadPart % freq.QuadPart) * 1000000 / freq.QuadPart);
  }
#endif
  return now;
}

Sorry, that's incomplete

@pgroke-dt
Copy link
Contributor Author

I personally wouldn't want to introduce a compile time switch for just that.

Any in your #if version, where do you init freq? If you can init freq without a race you can also init the "is vista" information the same way.

@pgroke-dt
Copy link
Contributor Author

pgroke-dt commented Feb 14, 2019 •
edited

How about

static LARGE_INTEGER qpcFreq;
static int useQpc;

void Curl_init_qpc() {
  if(Curl_verify_windows_version(6, 0, PLATFORM_WINNT,
                                 VERSION_GREATER_THAN_EQUAL)) {
    useQpc = QueryPerformanceFrequency(&qpcFreq) != 0;
  } else {
    useQpc = 0;
  }
}

and calling Curl_init_qpc from win32_init?

bagder added a commit that referenced this issue Feb 14, 2019
@bagder
Curl_now: figure out windows version in win32_init
2f1c0bf 
... and avoid use of static variables that aren't thread safe.

Fixes regression from e9ababd

Reported-by: Paul Groke
Fixes #3572
@bagder bagder mentioned this issue Feb 14, 2019
Closed
@bagder
Copy link
Member

bagder commented Feb 14, 2019

#3573 moves the initialization of the variables to the init function. I have not tested this, so I will certainly appreciate feedback and corrections if any!

@pgroke-dt
Copy link
Contributor Author

Looks good to me.

@bagder bagder closed this as completed in ca597ad Feb 15, 2019
@pps83
Copy link
Contributor

pps83 commented Feb 16, 2019

Sorry for coming late to the party ;)

I personally wouldn't want to introduce a compile time switch for just that.

I totally agree with that. In my project we totally have to support WinXP (heavy install base in China), but most of our users use modern Windows versions. Also, that's a minor impl detail to have compilation time change for that.
The change/fix is looks good, sorry for introducing regression.

@bagder
Copy link
Member

bagder commented Feb 16, 2019

No worries @pps83, several of us missed that. Thanks for checking out the fix!

@lock lock bot locked as resolved and limited conversation to collaborators May 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
crash Windows Windows-specific
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Curl_now: figure out windows version in win32_init curl/curl
4 participants
@bagder @pps83 @MarcelRaad @pgroke-dt