Code clean-up in AesDecryptor and test.

media/crypto/aes_decryptor.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/crypto/aes_decryptor.h"
 
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "crypto/encryptor.h"
@@ skipping 65 matching lines @@
 
   DCHECK(input.GetDecryptConfig()->checksum_size() <=
          static_cast<int>(hmac.DigestLength()));
   if (memcmp(input.GetDecryptConfig()->checksum(),
              calculated_hmac.get(),
              input.GetDecryptConfig()->checksum_size()) != 0)
     return false;
   return true;
 }
 
-// Decrypts |input| using |key|. |encrypted_data_offset| is the number of bytes
-// into |input| that the encrypted data starts.
-// Returns a DecoderBuffer with the decrypted data if decryption succeeded or
-// NULL if decryption failed.
-static scoped_refptr<DecoderBuffer> DecryptData(const DecoderBuffer& input,
-                                                crypto::SymmetricKey* key,
-                                                int encrypted_data_offset) {
-  CHECK(input.GetDataSize());
-  CHECK(input.GetDecryptConfig());
-  CHECK(key);
+// Decrypts |encrypted_data| using |key|.
+// Returns true and fill |decrypted_text| if if decryption succeeded.

[fgalligan1, 2012/07/25 19:33:50]

s/fill/fills/
s/if if/if/

[xhwang, 2012/07/26 23:36:47]

Done.

+// Returns false and leave |decrypted_text| as is if decryption failed.

[fgalligan1, 2012/07/25 19:33:50]

s/leave/leaves/

[ddorwin, 2012/07/26 21:24:07]

I think this is pretty common practice and thus does not need to be stated in
addition to 87.

[xhwang, 2012/07/26 23:36:47]

Done.

+static bool DecryptData(
+    const uint8* encrypted_data, int encrypted_data_size,
+    const uint8* iv, int iv_size,
+    crypto::SymmetricKey* key,
+    std::string* decrypted_text) {

[ddorwin, 2012/07/26 21:24:07]

nit: Odd that the output is a different type from the input.
Also odd that the names don't match (data vs. text).

[xhwang, 2012/07/26 23:36:47]

Done.

+  DCHECK(decrypted_text->empty());

[fgalligan1, 2012/07/25 19:33:50]

Why does decrypted_text need to be empty()?

[xhwang, 2012/07/26 23:36:47]

Done.

 
   // Initialize decryptor.
   crypto::Encryptor encryptor;
   if (!encryptor.Init(key, crypto::Encryptor::CTR, "")) {
     DVLOG(1) << "Could not initialize decryptor.";
-    return NULL;
+    return false;
   }
 
-  DCHECK_EQ(input.GetDecryptConfig()->iv_size(),
-            DecryptConfig::kDecryptionKeySize);
+  DCHECK_EQ(iv_size, DecryptConfig::kDecryptionKeySize);
   // Set the counter block.
-  base::StringPiece counter_block(
-      reinterpret_cast<const char*>(input.GetDecryptConfig()->iv()),
-      input.GetDecryptConfig()->iv_size());
+  base::StringPiece counter_block(reinterpret_cast<const char*>(iv),iv_size);

[ddorwin, 2012/07/26 21:24:07]

space after ,

[xhwang, 2012/07/26 23:36:47]

Done.

   if (counter_block.empty()) {
     DVLOG(1) << "Could not generate counter block.";
-    return NULL;
+    return false;
   }
   if (!encryptor.SetCounter(counter_block)) {
     DVLOG(1) << "Could not set counter block.";
-    return NULL;
+    return false;
   }
 
+  base::StringPiece encrypted_text(
+      reinterpret_cast<const char*>(encrypted_data), encrypted_data_size);
+  if (!encryptor.Decrypt(encrypted_text, decrypted_text)) {
+    DVLOG(1) << "Could not decrypt data.";
+    DCHECK(decrypted_text->empty());

[fgalligan1, 2012/07/25 19:33:50]

I don't think you need this check here if you remove the check above.

[xhwang, 2012/07/26 23:36:47]

Done.

+    return false;
+  }
+
+  DCHECK(!decrypted_text->empty());
+  return true;
+}
+
+// Decrypts |encrypted_buffer| using |key|.
+// Fires the |decrypt_cb| with kSuccess and the decrypted buffer if decryption
+// succeeded.
+// Fires the |decrypt_cb| with kError and NULL if decryption failed.

[ddorwin, 2012/07/26 21:24:07]

Otherwise fires ... NULL.

[xhwang, 2012/07/26 23:36:47]

Done.

+static void DecryptBuffer(const Decryptor::DecryptCB& decrypt_cb,
+                          const scoped_refptr<DecoderBuffer>& encrypted_buffer,
+                          crypto::SymmetricKey* key) {
+  CHECK(encrypted_buffer->GetDataSize());
+  CHECK(encrypted_buffer->GetDecryptConfig());
+  CHECK(key);
+
   std::string decrypted_text;
-  const char* frame =
-      reinterpret_cast<const char*>(input.GetData() + encrypted_data_offset);
-  int frame_size = input.GetDataSize() - encrypted_data_offset;
-  base::StringPiece encrypted_text(frame, frame_size);
-  if (!encryptor.Decrypt(encrypted_text, &decrypted_text)) {
-    DVLOG(1) << "Could not decrypt data.";
-    return NULL;
+  int encrypted_data_offset =
+      encrypted_buffer->GetDecryptConfig()->encrypted_frame_offset();
+
+  bool succeed = DecryptData(
+      encrypted_buffer->GetData() + encrypted_data_offset,
+      encrypted_buffer->GetDataSize() - encrypted_data_offset,
+      encrypted_buffer->GetDecryptConfig()->iv(),
+      encrypted_buffer->GetDecryptConfig()->iv_size(),
+      key,
+      &decrypted_text);
+
+  if (!succeed) {
+    DVLOG(1) << "Decryption failed.";
+    decrypt_cb.Run(Decryptor::kError, NULL);
+    return;
   }
 
   // TODO(xhwang): Find a way to avoid this data copy.
-  return DecoderBuffer::CopyFrom(
+  scoped_refptr<DecoderBuffer> decrypted_buffer = DecoderBuffer::CopyFrom(
       reinterpret_cast<const uint8*>(decrypted_text.data()),
       decrypted_text.size());
+
+  decrypted_buffer->SetTimestamp(encrypted_buffer->GetTimestamp());
+  decrypted_buffer->SetDuration(encrypted_buffer->GetDuration());
+  decrypt_cb.Run(Decryptor::kSuccess, decrypted_buffer);
 }
 
 AesDecryptor::AesDecryptor(DecryptorClient* client)
     : client_(client) {
 }
 
 AesDecryptor::~AesDecryptor() {
   STLDeleteValues(&key_map_);
 }
 
@@ skipping 49 matching lines @@
   }
 
   // TODO(fgalligan): When ISO is added we will need to figure out how to
   // detect if the encrypted data will contain an HMAC.
   if (!decryption_key->Init(true)) {
     DVLOG(1) << "Could not initialize decryption key.";
     client_->KeyError(key_system, session_id, Decryptor::kUnknownError, 0);
     return;
   }
 
-  {
-    base::AutoLock auto_lock(key_map_lock_);
-    KeyMap::iterator found = key_map_.find(key_id_string);
-    if (found != key_map_.end()) {
-      delete found->second;
-      key_map_.erase(found);
-    }
-    key_map_[key_id_string] = decryption_key.release();
-  }
-
+  SetKey(key_id_string, decryption_key.Pass());
   client_->KeyAdded(key_system, session_id);
 }
 
 void AesDecryptor::CancelKeyRequest(const std::string& key_system,
                                     const std::string& session_id) {
 }
 
 void AesDecryptor::Decrypt(const scoped_refptr<DecoderBuffer>& encrypted,

[ddorwin, 2012/07/26 21:24:07]

"encrypted" isn't really a noun. I realize changing it will make this not fit (
:( ).

[xhwang, 2012/07/26 23:36:47]

Chatted with ddorwin@ offline. We'll use:
a) "encrypted_data" for string, StringPiece and scoped_array<uint8> and uint8*,
b) "encrypted_buffer" for scoped_refptr<DecoderBuffer>.

I'll make the change in another CL though.

                            const DecryptCB& decrypt_cb) {
   CHECK(encrypted->GetDecryptConfig());
   const uint8* key_id = encrypted->GetDecryptConfig()->key_id();
   const int key_id_size = encrypted->GetDecryptConfig()->key_id_size();
 
   // TODO(xhwang): Avoid always constructing a string with StringPiece?
   std::string key_id_string(reinterpret_cast<const char*>(key_id), key_id_size);
-
-  DecryptionKey* key = NULL;
-  {
-    base::AutoLock auto_lock(key_map_lock_);
-    KeyMap::const_iterator found = key_map_.find(key_id_string);
-    if (found != key_map_.end())
-      key = found->second;
-  }
+  DecryptionKey* key = GetKey(key_id_string);
 
   if (!key) {
-    // TODO(fgalligan): Fire a need_key event here and add a test.
-    DVLOG(1) << "Could not find a matching key for given key ID.";
+    DVLOG(1) << "Could not find a matching key for the given key ID.";

[ddorwin, 2012/07/26 21:24:07]

This is still temporary, right? So there should still be a TODO (maybe for
xhwang).

[xhwang, 2012/07/26 23:36:47]

Done.

     decrypt_cb.Run(kError, NULL);
     return;
   }
 
   int checksum_size = encrypted->GetDecryptConfig()->checksum_size();
   // According to the WebM encrypted specification, it is an open question
   // what should happen when a frame fails the integrity check.
   // http://wiki.webmproject.org/encryption/webm-encryption-rfc
   if (checksum_size > 0 &&
       !key->hmac_key().empty() &&
       !CheckData(*encrypted, key->hmac_key())) {
     DVLOG(1) << "Integrity check failed.";
     decrypt_cb.Run(kError, NULL);
     return;
   }
 
-  scoped_refptr<DecoderBuffer> decrypted =
-      DecryptData(*encrypted,
-                  key->decryption_key(),
-                  encrypted->GetDecryptConfig()->encrypted_frame_offset());
-  if (!decrypted) {
-    DVLOG(1) << "Decryption failed.";
-    decrypt_cb.Run(kError, NULL);
-    return;
-  }
-
-  decrypted->SetTimestamp(encrypted->GetTimestamp());
-  decrypted->SetDuration(encrypted->GetDuration());
-  decrypt_cb.Run(kSuccess, decrypted);
+  DecryptBuffer(decrypt_cb, encrypted, key->decryption_key());
 }
 
-AesDecryptor::DecryptionKey::DecryptionKey(
-    const std::string& secret)
+void AesDecryptor::SetKey(const std::string& key_id,
+                          scoped_ptr<DecryptionKey> decryption_key) {

[ddorwin, 2012/07/26 21:24:07]

Why not just a raw pointer with a note in .h that this function takes ownership?

[xhwang, 2012/07/26 23:36:47]

Passing in scoped_ptr<> enforces that the AesDecryptor takes the ownership,
which IMHO is better.

+  base::AutoLock auto_lock(key_map_lock_);
+  KeyMap::iterator found = key_map_.find(key_id);
+  if (found != key_map_.end()) {
+    delete found->second;
+    key_map_.erase(found);
+  }
+  key_map_[key_id] = decryption_key.release();
+}
+
+AesDecryptor::DecryptionKey* AesDecryptor::GetKey(
+    const std::string& key_id) const {
+  base::AutoLock auto_lock(key_map_lock_);
+  KeyMap::const_iterator found = key_map_.find(key_id);
+  if (found != key_map_.end())

[ddorwin, 2012/07/26 21:24:07]

I'd swap such that the success case is at the end of the function. (That's a
more common pattern and uses == instead of !=.)

[xhwang, 2012/07/26 23:36:47]

Done.

+    return found->second;
+  return NULL;
+}
+
+AesDecryptor::DecryptionKey::DecryptionKey(const std::string& secret)
     : secret_(secret) {
 }
 
 AesDecryptor::DecryptionKey::~DecryptionKey() {}
 
 bool AesDecryptor::DecryptionKey::Init(bool derive_webm_keys) {
   CHECK(!secret_.empty());
 
   if (derive_webm_keys) {
     std::string raw_key = DeriveKey(secret_,
@@ skipping 17 matching lines @@
 
   decryption_key_.reset(
       crypto::SymmetricKey::Import(crypto::SymmetricKey::AES, secret_));
   if (!decryption_key_.get()) {
     return false;
   }
   return true;
 }
 
 }  // namespace media