src: avoid heap allocation in hmac.digest()

Add a test that ensures the second call to .digest() returns an empty HMAC, like it did before. No comment on whether that is the right behavior or not.
nodejs · bnoordhuis · Jul 10, 2017 · Jul 10, 2017 · Jul 10, 2017 · Jul 10, 2017
commit 496ef4b0c2377232892f0610e4a65de849e3cdd2
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
@@ -3767,17 +3767,6 @@ void Hmac::HmacUpdate(const FunctionCallbackInfo<Value>& args) {
 }
 
 
-bool Hmac::HmacDigest(unsigned char** md_value, unsigned int* md_len) {
-  if (!initialised_)
-    return false;
-  *md_value = new unsigned char[EVP_MAX_MD_SIZE];
-  HMAC_Final(&ctx_, *md_value, md_len);
-  HMAC_CTX_cleanup(&ctx_);
-  initialised_ = false;
-  return true;
-}
-
-
 void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
@@ -3794,13 +3783,13 @@ void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
     return env->ThrowError("hmac.digest() does not support UTF-16");
   }
 
-  unsigned char* md_value = nullptr;
+  unsigned char md_value[EVP_MAX_MD_SIZE];
   unsigned int md_len = 0;
 
-  bool r = hmac->HmacDigest(&md_value, &md_len);
-  if (!r) {
-    md_value = nullptr;
-    md_len = 0;
+  if (hmac->initialised_) {
+    HMAC_Final(&hmac->ctx_, md_value, &md_len);
+    HMAC_CTX_cleanup(&hmac->ctx_);
+    hmac->initialised_ = false;
   }
 
   Local<Value> error;
@@ -3810,7 +3799,6 @@ void Hmac::HmacDigest(const FunctionCallbackInfo<Value>& args) {
                           md_len,
                           encoding,
                           &error);
-  delete[] md_value;
   if (rc.IsEmpty()) {
     CHECK(!error.IsEmpty());
     env->isolate()->ThrowException(error);

diff --git a/src/node_crypto.h b/src/node_crypto.h
@@ -494,7 +494,6 @@ class Hmac : public BaseObject {
  protected:
   void HmacInit(const char* hash_type, const char* key, int key_len);
   bool HmacUpdate(const char* data, int len);
-  bool HmacDigest(unsigned char** md_value, unsigned int* md_len);
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void HmacInit(const v8::FunctionCallbackInfo<v8::Value>& args);

diff --git a/test/parallel/test-crypto-hmac.js b/test/parallel/test-crypto-hmac.js
@@ -379,3 +379,38 @@ for (let i = 0, l = rfc2202_sha1.length; i < l; i++) {
 assert.throws(function() {
   crypto.createHmac('sha256', 'w00t').digest('ucs2');
 }, /^Error: hmac\.digest\(\) does not support UTF-16$/);
+
+// Check initialized -> uninitialized state transition after calling digest().
+{
+  const expected =
+      '\u0010\u0041\u0052\u00c5\u00bf\u00dc\u00a0\u007b\u00c6\u0033' +
+      '\u00ee\u00bd\u0046\u0019\u009f\u0002\u0055\u00c9\u00f4\u009d';
+  {
+    const h = crypto.createHmac('sha1', 'key').update('data');
+    assert.deepStrictEqual(h.digest('buffer'), Buffer.from(expected, 'latin1'));
+    assert.deepStrictEqual(h.digest('buffer'), Buffer.from(''));
+  }
+  {
+    const h = crypto.createHmac('sha1', 'key').update('data');
+    assert.deepStrictEqual(h.digest('latin1'), expected);
+    assert.deepStrictEqual(h.digest('latin1'), '');
+  }
+}
+
+// Check initialized -> uninitialized state transition after calling digest().
+// Calls to update() omitted intentionally.
+{
+  const expected =
+      '\u00f4\u002b\u00b0\u00ee\u00b0\u0018\u00eb\u00bd\u0045\u0097' +
+      '\u00ae\u0072\u0013\u0071\u001e\u00c6\u0007\u0060\u0084\u003f';
+  {
+    const h = crypto.createHmac('sha1', 'key');
+    assert.deepStrictEqual(h.digest('buffer'), Buffer.from(expected, 'latin1'));
+    assert.deepStrictEqual(h.digest('buffer'), Buffer.from(''));
+  }
+  {
+    const h = crypto.createHmac('sha1', 'key');
+    assert.deepStrictEqual(h.digest('latin1'), expected);
+    assert.deepStrictEqual(h.digest('latin1'), '');
+  }
+}