Skip to content

Division by zero in Curl_now #3572

Closed
Closed
Division by zero in Curl_now#3572
Labels
WindowsWindows-specificcrash

Description

@pgroke-dt
pgroke-dt
opened on Feb 14, 2019

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.

Activity

  1. bagder
    added
    crash
    WindowsWindows-specific
    on Feb 14, 2019

  2. bagder commented on Feb 14, 2019

    @bagder
    bagder
    on Feb 14, 2019
    Member

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

  3. bagder commented on Feb 14, 2019

    @bagder
    bagder
    on Feb 14, 2019
    Member

    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.

  4. pgroke-dt commented on Feb 14, 2019

    @pgroke-dt
    pgroke-dt
    on Feb 14, 2019
    ContributorAuthor

    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 {
...

  5. MarcelRaad commented on Feb 14, 2019

    @MarcelRaad
    MarcelRaad
    on Feb 14, 2019
    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 :-)

  6. bagder commented on Feb 14, 2019

    @bagder
    bagder
    on Feb 14, 2019
    Member

    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.

  7. pgroke-dt commented on Feb 14, 2019

    @pgroke-dt
    pgroke-dt
    on Feb 14, 2019
    ContributorAuthor

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

  8. bagder commented on Feb 14, 2019

    @bagder
    bagder
    on Feb 14, 2019
    Member

    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

  9. pgroke-dt commented on Feb 14, 2019

    @pgroke-dt
    pgroke-dt
    on Feb 14, 2019
    ContributorAuthor

    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.

  10. pgroke-dt commented on Feb 14, 2019

    @pgroke-dt
    pgroke-dt
    on Feb 14, 2019
    ContributorAuthor

    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?

  11. bagder
    added a commit that references this issue on Feb 14, 2019

    Curl_now: figure out windows version in win32_init

    2f1c0bf

  12. bagder commented on Feb 14, 2019

    @bagder
    bagder
    on Feb 14, 2019
    Member

    #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!

  13. pgroke-dt commented on Feb 14, 2019

    @pgroke-dt
    pgroke-dt
    on Feb 14, 2019
    ContributorAuthor

    Looks good to me.

  14. bagder
    closed this as completedin ca597adon Feb 15, 2019

  15. pps83 commented on Feb 16, 2019

    @pps83
    pps83
    on Feb 16, 2019
    Contributor

    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.

  16. bagder commented on Feb 16, 2019

    @bagder
    bagder
    on Feb 16, 2019
    Member

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

  17. lock
    locked as resolved and limited conversation to collaborators on May 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    WindowsWindows-specificcrash

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions