Unify the error checking of crypto::Encryptor and add WARN_UNUSED_RESULT to prevent misuse.

crypto/encryptor_nss.cc

Index: crypto/encryptor_nss.cc
diff --git a/crypto/encryptor_nss.cc b/crypto/encryptor_nss.cc
index c46109031ac88093ac70cf863c389faa53a16c9c..0f8dbc8144bd7a76c26ba32d9cd3aa14afb4e585 100644
--- a/crypto/encryptor_nss.cc
+++ b/crypto/encryptor_nss.cc
@@ -44,8 +44,8 @@ Encryptor::~Encryptor() {
 bool Encryptor::Init(SymmetricKey* key,
                      Mode mode,
                      const base::StringPiece& iv) {
-  DCHECK(key);
-  DCHECK(CBC == mode || CTR == mode) << "Unsupported mode of operation";
+  if (!key || !(mode == CBC || mode == CTR))
+    return false;
 
   key_ = key;
   mode_ = mode;
@@ -72,28 +72,29 @@ bool Encryptor::Init(SymmetricKey* key,
       break;
   }
 
-  if (!param_.get())
-    return false;
-  return true;
+  return param_ != NULL;
 }
 
 bool Encryptor::Encrypt(const base::StringPiece& plaintext,
                         std::string* ciphertext) {
-  ScopedPK11Context context(PK11_CreateContextBySymKey(GetMechanism(mode_),
-                                                       CKA_ENCRYPT,
-                                                       key_->key(),
-                                                       param_.get()));
+  ciphertext->clear();
+  if (plaintext.empty() && mode_ != CBC)
+    return false;
+
+  ScopedPK11Context context(
+      PK11_CreateContextBySymKey(GetMechanism(mode_), CKA_ENCRYPT,
+                                 key_->key(), param_.get()));
   if (!context.get())
     return false;
 
-  if (mode_ == CTR)
-    return CryptCTR(context.get(), plaintext, ciphertext);
-  else
-    return Crypt(context.get(), plaintext, ciphertext);
+  return (mode_ == CTR) ?
+      CryptCTR(context.get(), plaintext, ciphertext) :
+      Crypt(context.get(), plaintext, ciphertext);
 }
 
 bool Encryptor::Decrypt(const base::StringPiece& ciphertext,
                         std::string* plaintext) {
+  plaintext->clear();
   if (ciphertext.empty())
     return false;
 
@@ -103,50 +104,41 @@ bool Encryptor::Decrypt(const base::StringPiece& ciphertext,
   if (!context.get())
     return false;
 
-  if (mode_ == CTR)
-    return CryptCTR(context.get(), ciphertext, plaintext);
-  else
-    return Crypt(context.get(), ciphertext, plaintext);
+  return (mode_ == CTR) ?
+      CryptCTR(context.get(), ciphertext, plaintext) :
+      Crypt(context.get(), ciphertext, plaintext);
 }
 
 bool Encryptor::Crypt(PK11Context* context,
                       const base::StringPiece& input,
                       std::string* output) {
   size_t output_len = input.size() + AES_BLOCK_SIZE;
-  CHECK(output_len > input.size()) << "Output size overflow";
+  if (output_len < input.size())
+    return false;  // Size overflow.

[wtc, 2011/11/15 02:33:58]

Nit: Size => size_t

 
-  output->resize(output_len);
-  uint8* output_data =
-      reinterpret_cast<uint8*>(const_cast<char*>(output->data()));
+  std::string result;
+  result.resize(output_len);

[wtc, 2011/11/15 02:33:58]

Is there a constructor that takes the size input?

+  uint8* result_data =
+      reinterpret_cast<uint8*>(const_cast<char*>(result.data()));
 
   int input_len = input.size();
   uint8* input_data =
       reinterpret_cast<uint8*>(const_cast<char*>(input.data()));
 
   int op_len;
-  SECStatus rv = PK11_CipherOp(context,
-                               output_data,
-                               &op_len,
-                               output_len,
-                               input_data,
-                               input_len);
-
-  if (SECSuccess != rv) {
-    output->clear();
+  SECStatus rv = PK11_CipherOp(context, result_data, &op_len, output_len,
+                               input_data, input_len);
+  if (SECSuccess != rv)
     return false;
-  }
 
   unsigned int digest_len;
-  rv = PK11_DigestFinal(context,
-                        output_data + op_len,
-                        &digest_len,
+  rv = PK11_DigestFinal(context, result_data + op_len, &digest_len,
                         output_len - op_len);
-  if (SECSuccess != rv) {
-    output->clear();
+  if (SECSuccess != rv)
     return false;
-  }
 
-  output->resize(op_len + digest_len);
+  result.resize(op_len + digest_len);
+  output->swap(result);
   return true;
 }
 
@@ -160,42 +152,38 @@ bool Encryptor::CryptCTR(PK11Context* context,
 
   size_t output_len = ((input.size() + AES_BLOCK_SIZE - 1) / AES_BLOCK_SIZE) *
       AES_BLOCK_SIZE;
-  CHECK(output_len >= input.size()) << "Output size overflow";
-  output->resize(output_len);
-  uint8* output_data =
-      reinterpret_cast<uint8*>(const_cast<char*>(output->data()));
+  if (output_len < input.size())
+    return false;  // Size overflow.
+
+  // Work on the result in a local variable, and then only transfer it to
+  // |output| on success to ensure no partial data is returned.
+  std::string result;
+  result.resize(output_len);
+  uint8* result_data =
+      reinterpret_cast<uint8*>(const_cast<char*>(result.data()));
 
   size_t mask_len;
-  bool ret = GenerateCounterMask(input.size(), output_data, &mask_len);
+  bool ret = GenerateCounterMask(input.size(), result_data, &mask_len);
   if (!ret)
     return false;
 
   CHECK_EQ(mask_len, output_len);
   int op_len;
-  SECStatus rv = PK11_CipherOp(context,
-                               output_data,
-                               &op_len,
-                               output_len,
-                               output_data,
-                               mask_len);
-  if (SECSuccess != rv)
+  SECStatus rv = PK11_CipherOp(context, result_data, &op_len, output_len,
+                               result_data, mask_len);
+  if (SECSuccess != rv || op_len != static_cast<int>(mask_len))
     return false;
-  CHECK(op_len == static_cast<int>(mask_len));
 
   unsigned int digest_len;
-  rv = PK11_DigestFinal(context,
-                        NULL,
-                        &digest_len,
-                        0);
-  if (SECSuccess != rv)
+  rv = PK11_DigestFinal(context, NULL, &digest_len, 0);
+  if (SECSuccess != rv || digest_len)
     return false;
-  CHECK(!digest_len);

[wtc, 2011/11/15 02:33:58]

IMPORTANT: Some of these CHECKs should not be converted to tests.
If they are not useful, we can remove them.

 
-  // Use |output_data| to mask |input|.
-  MaskMessage(
-      reinterpret_cast<uint8*>(const_cast<char*>(input.data())),
-      input.length(), output_data, output_data);
-  output->resize(input.length());
+  // Use |result_data| to mask |input|.
+  MaskMessage(reinterpret_cast<uint8*>(const_cast<char*>(input.data())),
+              input.size(), result_data, result_data);
+  result.resize(input.size());
+  output->swap(result);
   return true;
 }