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

crypto/encryptor_mac.cc

 // Copyright (c) 2011 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 "crypto/encryptor.h"
 
 #include <CommonCrypto/CommonCryptor.h>
 
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "crypto/symmetric_key.h"
 
 namespace crypto {
 
 Encryptor::Encryptor()
     : key_(NULL),
       mode_(CBC) {
 }
 
 Encryptor::~Encryptor() {
 }
 
 bool Encryptor::Init(SymmetricKey* key,
                      Mode mode,
                      const base::StringPiece& iv) {
-  DCHECK(key);
-  DCHECK_EQ(CBC, mode) << "Unsupported mode of operation";
+  DCHECK_EQ(CBC, mode);
+  if (!key)
+    return false;
+
   CSSM_DATA raw_key = key->cssm_data();
   if (raw_key.Length != kCCKeySizeAES128 &&
       raw_key.Length != kCCKeySizeAES192 &&
       raw_key.Length != kCCKeySizeAES256)
     return false;
   if (iv.size() != kCCBlockSizeAES128)
     return false;
 
   key_ = key;
   mode_ = mode;
   iv.CopyToString(&iv_);
   return true;
 }
 
 bool Encryptor::Crypt(int /*CCOperation*/ op,
                       const base::StringPiece& input,
                       std::string* output) {
-  DCHECK(key_);
+  std::string result;
+  output->swap(result);

[wtc, 2011/12/15 02:03:58]

IMPORTANT: this swap is not necessary.  Our convention is
that if a function fails, the output arguments should be
ignored.

[Peter Kasting, 2011/12/15 02:12:39]

FWIW I intentionally eliminated both swap()s in my change and replaced them with
direct modification of |output|, which I still think is clearer and cleaner than
the old way that this patch returns to.

I'm not sure I agree that we have a consistent convention about how to deal with
outparams on failure; I thought the convention (if any) was that if easy, code
should leave them untouched rather than explicitly clearing them as this code
does.  This could be implemented by simply eliminating the first swap() (here)
in this function, which I think would be an improvement if we really don't want
to touch |output| in the rest of the function body.

[wtc, 2011/12/15 02:16:44]

You are right.  You stated the convention better than I did.

[Ryan Sleevi, 2011/12/15 02:18:16]

This was based on past misuses of this API, where failure was ignored. Even with
WARN_UNUSED_RESULT, it's easy to miss checking the result:
bool b = encryptor_.Encrypt(plaintext, &ciphertext);
// Do something with ciphertext - except perhaps bad.

The effect this has is that it's equivalent to output->clear(). By using
->swap(result), we can re-use output's pointer for result (which is generally
pre-allocated/sized to the right length).

I'm fine removing this extra layer of misuse guarding, and writing everything
into |output| (and then needing to ->clear() on any subsequent / API failures),
I just wanted to make sure the reasoning for it was known.

[wtc, 2011/12/15 22:04:14]

Thank you for the explanation.  It is hard to figure out
what the extra output->swap(result) call at the beginning
of this function is intended for.  I would just remove it
rather than adding a comment to explain it.

+  if (!key_)
+    return false;
+
   CSSM_DATA raw_key = key_->cssm_data();
   // CommonCryptor.h: "A general rule for the size of the output buffer which
   // must be provided by the caller is that for block ciphers, the output
   // length is never larger than the input length plus the block size."
-
-  size_t output_size = input.size() + iv_.size();
-  CHECK_GT(output_size, 0u);
-  CHECK_GT(output_size + 1, input.size());
+  size_t result_size = input.size() + iv_.size();
+  if (result_size == 0 || result_size < input.size() ||
+      result_size + 1 < input.size()) {

[wtc, 2011/12/15 02:03:58]

IMPORTANT: The need to test both result_size < input.size() and
result_size + 1 < input.size() is too subtle.  If it is
intentional, please add a comment.

[Ryan Sleevi, 2011/12/15 02:18:16]

It is intentional - I will add a comment to clarify.

[Peter Kasting, 2011/12/15 02:20:09]

Uff da.

If you're going to add a comment anyway, then please condense to only checking
the "+ 1" case in code.  You can mention the other case in your comment and talk
about what it means and that the + 1 case will cover it.

+    return false;
+  }
   CCCryptorStatus err = CCCrypt(op,
                                 kCCAlgorithmAES128,
                                 kCCOptionPKCS7Padding,
                                 raw_key.Data, raw_key.Length,
                                 iv_.data(),
                                 input.data(), input.size(),
-                                WriteInto(output, output_size + 1),
-                                output_size,
-                                &output_size);
+                                WriteInto(&result, result_size + 1),
+                                result_size,
+                                &result_size);
   if (err) {
-    output->clear();
     LOG(ERROR) << "CCCrypt returned " << err;
     return false;
   }
-  output->resize(output_size);
+  result.resize(result_size);
+  output->swap(result);
   return true;
 }
 
 bool Encryptor::Encrypt(const base::StringPiece& plaintext,
                         std::string* ciphertext) {
-  CHECK(!plaintext.empty() || (mode_ == CBC));
+  if (plaintext.empty() && mode_ != CBC) {
+    ciphertext->clear();

[wtc, 2011/12/15 02:03:58]

This ciphertext->clear() call and the one on line 91 below
can be omitted.

+    return false;
+  }
   return Crypt(kCCEncrypt, plaintext, ciphertext);
 }
 
 bool Encryptor::Decrypt(const base::StringPiece& ciphertext,
                         std::string* plaintext) {
-  CHECK(!ciphertext.empty());
+  if (ciphertext.empty()) {
+    plaintext->clear();
+    return false;
+  }
   return Crypt(kCCDecrypt, ciphertext, plaintext);
 }
 
 }  // namespace crypto