{"id":92934,"date":"2016-01-27T07:00:00","date_gmt":"2016-01-27T22:00:00","guid":{"rendered":"https:\/\/blogs.msdn.microsoft.com\/oldnewthing\/?p=92934"},"modified":"2019-03-13T10:29:21","modified_gmt":"2019-03-13T17:29:21","slug":"20160127-00","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/oldnewthing\/20160127-00\/?p=92934","title":{"rendered":"Why does CryptDestroyHash crash, but only sometimes?"},"content":{"rendered":"<p>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 <code>Crypt&shy;Destroy&shy;Hash<\/code>: <\/p>\n<pre>\nbool SomethingSomething(BYTE *buffer, int bufferSize)\n{\n    bool succeeded = true;\n    HCRYPTPROV provider = 0;\n    HCRYPTHASH hash = 0;\n\n    if (!CryptAcquireContext(&amp;provider, NULL, NULL,\n                           PROV_RSA_FULL, CRYPT_VERIFYCONTEXT) ||\n        !CryptCreateHash(provider, CALG_MD5, 0, 0, &amp;hash))\n    {\n        succeeded = false;\n        goto Exit;\n    }\n\n    BYTE hashResult[16]; \/\/ MD5 hash is 16 bytes\n    DWORD hashResultSize = sizeof(hashResult);\n\n    if (!CryptHashData(hash, buffer, bufferSize, 0) ||\n        !CryptGetHashParam(hash, HP_HASHVAL, hashResult,\n                                          &amp;hashResultSize, 0)) {\n        succeeded = false;\n        goto Exit;\n    }\n\n    DoSomethingWith(hashResult); \/\/ some business logic\n\n    if (provider) {\n        CryptReleaseContext(provider, 0);\n    }\n\n    if (hash) {\n        CryptDestroyHash(hash);\n    }\n\nExit:\n\n    return succeeded;\n}\n<\/pre>\n<p>The reason for the crash is straightforward. As noted in the documentation, <a HREF=\"https:\/\/msdn.microsoft.com\/en-us\/library\/windows\/desktop\/aa380268(v=vs.85).aspx\">you must call <code>Crypt&shy;Destroy&shy;Hash<\/code> before <code>Crypt&shy;Release&shy;Context<\/code><\/a>. (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.) <\/p>\n<p>This was a relatively straightforward consult. A simple programming error. The customer thanked us for identifying the problem, but then followed up with &#8220;But why is it happening only rarely? Shouldn&#8217;t it crash all the time?&#8221; <\/p>\n<p>Remember that when you break the rules, the behavior is undefined, and one valid manifestion of undefined behavior is &#8220;Everything seems to work okay.&#8221; <\/p>\n<p>You may have noticed some other problems with the code provided. <\/p>\n<ul>\n<li>If anything goes wrong, the calls to     <code>Crypt&shy;Destroy&shy;Hash<\/code> and     <code>Crypt&shy;Release&shy;Context<\/code> are skipped,     which means that the code leaks a hash and a context.     The <code>Exit<\/code> label should be moved to just     in front of the <code>if (provider)<\/code>. \n<li>Setting <code>succeeded = true<\/code> and then manually setting     it to <code>false<\/code> when something goes wrong strikes me     as a high-risk proposition.     If somebody adds code to the function and does a     <code>goto Exit;<\/code> without also setting     <code>succeeded = false;<\/code>, the function will falsely     report success.     I prefer to fail safe and initialize     <code>succeeded = false;<\/code>, and set it to <code>true<\/code>     only after I am sure that the function succeeded. <\/ul>\n<p>Using RAII would have solved both the order-of-destruction problem and the memory leaks. <\/p>\n<pre>\nbool SomethingSomething(BYTE *buffer, int bufferSize)\n{\n    \/\/ assuming suitable definitions for CryptProv and CryptHash\n    CryptProv provider(NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT);\n    if (!provider) return false;\n    CryptHash hash(provider.get(), CALG_MD5, 0, 0);\n    if (!hash) return false;\n\n    BYTE hashResult[16]; \/\/ MD5 hash is 16 bytes\n    DWORD hashResultSize = sizeof(hashResult);\n\n    if (!CryptHashData(hash.get(), buffer, bufferSize, 0) ||\n        !CryptGetHashParam(hash.get(), HP_HASHVAL, hashResult,\n                                          &amp;hashResultSize, 0)) {\n        return false;\n    }\n\n    DoSomethingWith(hashResult); \/\/ some business logic\n\n    return true;\n}\n<\/pre>\n","protected":false},"excerpt":{"rendered":"<p>Improper destruction leads to undefined behavior, and undefined behavior includes &#8220;crashing only sometimes&#8221;.<\/p>\n","protected":false},"author":1069,"featured_media":111744,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[1],"tags":[25],"class_list":["post-92934","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-oldnewthing","tag-code"],"acf":[],"blog_post_summary":"<p>Improper destruction leads to undefined behavior, and undefined behavior includes &#8220;crashing only sometimes&#8221;.<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/92934","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/users\/1069"}],"replies":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/comments?post=92934"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/92934\/revisions"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media\/111744"}],"wp:attachment":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media?parent=92934"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/categories?post=92934"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/tags?post=92934"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}