Why does CryptDestroyHash crash, but only sometimes?


A customer was having a problem with the cryptographic hashing functions. They reported that their function ran successfully most of the time, but once in a while, it crashed at the call to Crypt­Destroy­Hash:

bool SomethingSomething(BYTE *buffer, int bufferSize)
{
    bool succeeded = true;
    HCRYPTPROV provider = 0;
    HCRYPTHASH hash = 0;

    if (!CryptAcquireContext(&provider, NULL, NULL,
                           PROV_RSA_FULL, CRYPT_VERIFYCONTEXT) ||
        !CryptCreateHash(provider, CALG_MD5, 0, 0, &hash))
    {
        succeeded = false;
        goto Exit;
    }

    BYTE hashResult[16]; // MD5 hash is 16 bytes
    DWORD hashResultSize = sizeof(hashResult);

    if (!CryptHashData(hash, buffer, bufferSize, 0) ||
        !CryptGetHashParam(hash, HP_HASHVAL, hashResult,
                                          &hashResultSize, 0)) {
        succeeded = false;
        goto Exit;
    }

    DoSomethingWith(hashResult); // some business logic

    if (provider) {
        CryptReleaseContext(provider, 0);
    }

    if (hash) {
        CryptDestroyHash(hash);
    }

Exit:

    return succeeded;
}

The reason for the crash is straightforward. As noted in the documentation, you must call Crypt­Destroy­Hash before Crypt­Release­Context. (The technical reason for this is that each hash has a reference back to the context, so if you destroy the context, you leave the hash with a dangling pointer.)

This was a relatively straightforward consult. A simple programming error. The customer thanked us for identifying the problem, but then followed up with "But why is it happening only rarely? Shouldn't it crash all the time?"

Remember that when you break the rules, the behavior is undefined, and one valid manifestion of undefined behavior is "Everything seems to work okay."

You may have noticed some other problems with the code provided.

  • If anything goes wrong, the calls to Crypt­Destroy­Hash and Crypt­Release­Context are skipped, which means that the code leaks a hash and a context. The Exit label should be moved to just in front of the if (provider).

  • Setting succeeded = true and then manually setting it to false when something goes wrong strikes me as a high-risk proposition. If somebody adds code to the function and does a goto Exit; without also setting succeeded = false;, the function will falsely report success. I prefer to fail safe and initialize succeeded = false;, and set it to true only after I am sure that the function succeeded.

Using RAII would have solved both the order-of-destruction problem and the memory leaks.

bool SomethingSomething(BYTE *buffer, int bufferSize)
{
    // assuming suitable definitions for CryptProv and CryptHash
    CryptProv provider(NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT);
    if (!provider) return false;
    CryptHash hash(provider.get(), CALG_MD5, 0, 0);
    if (!hash) return false;

    BYTE hashResult[16]; // MD5 hash is 16 bytes
    DWORD hashResultSize = sizeof(hashResult);

    if (!CryptHashData(hash.get(), buffer, bufferSize, 0) ||
        !CryptGetHashParam(hash.get(), HP_HASHVAL, hashResult,
                                          &hashResultSize, 0)) {
        return false;
    }

    DoSomethingWith(hashResult); // some business logic

    return true;
}
Comments (10)
  1. Brian says:

    I hadn't realized that the order objects were destroyed in was guaranteed by the standard - thanks for the pointer. For those curious, this StackOverflow post goes into the details a bit more: http://stackoverflow.com/questions/14688285/c-local-variable-destruction-order

    1. Even further, [class.base.init](10) says "non-static data members [of a class] are initialized in the order they were declared in the class definition" (note! not the order of initialization in the constructor!) and [class.dtor](8) says that "Bases and members are destroyed in the reverse order of the completion of their constructor" so they destruct in reverse order of declaration.

      1. Compiler non-standard exception feature request: allow me to enforce that bases/members in the initializer list are in the same order as the class definition.

        1. GregM says:

          Clang and GCC already do this.

          1. Cesar says:

            > Clang and GCC already do this.

            Only if you use -Werror, since it's a warning. I'd expect it to be the same in MSVC (a warning which can be turned into an error in the command line).

    2. McBucket says:

      This is a huge feature in C++ and has been for a long time: reliable construction/destruction order of objects is critical to making resource lifetime management (memory, files, handles, whatever) more, well, manageable for us humans. This can make awkward error-handling a lot easier for the programmer, as when you need to use multiple resources in a function, and acquiring them may not be guaranteed, Use classes that acquires in the constructors, and releases in the destructors, and you can make error handling cleanup as simple as returning from the function (or throwing an exception) at the spot where a problem is encountered. I sometimes say that '}' is my favorite C++ 'operator'.

  2. ipoverscsi says:

    Raymond: it's been some time since I looked at the C++ specification, but is it really guaranteed that objects will be destructed in the opposite order of construction within an activation record? This would have to been the case for the assertion that "RAII would have solved [...] the order-of-destruction problem"

    1. Joshua says:

      It does. How nice!

    2. Darran Rowe says:

      It is in there with the class destructors.
      There is an unconditional statement that bases and members are destroyed in the reverse order of construction. See 12.4 paragraph 8, unless it has changed in the interim.

    3. Neil says:

      Although temporaries used in the construction of a variable (or temporary with an extended lifetime) will get constructed before that variable but also destroyed before that variable is destroyed. (You can use this to detect whether a RAII class got assigned to a variable or just created incorrectly as a temporary by mistake.)

Comments are closed.

Skip to main content