Use base::StringPiece for input parameters in Encryptor, rather than std::string

Use base::StringPiece for input parameters in Encryptor, rather than std::string

R=wtc
BUG=none
TEST=crypto_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91800

[Ryan Sleevi, 2011-06-28 04:47:27]

wtc: PTAL.

I noticed in http://crrev.com/90606 that there was a TODO(hclam) about changing
the Encryptor API to avoid an extra memcpy, and using base::StringPiece seemed
the appropriate thing here. I directly included the header so as to reduce the
number of callers that needed to be updated (thus keeping the CL small), but if
you'd rather I forward declare and update all the callsites, let me know.

hclam: FYI.

[wtc, 2011-06-28 22:42:09]

http://codereview.chromium.org/7230037/diff/2003/crypto/encryptor.h
File crypto/encryptor.h (right):

http://codereview.chromium.org/7230037/diff/2003/crypto/encryptor.h#newcode71
crypto/encryptor.h:71: bool Decrypt(const base::StringPiece& ciphertext,
std::string* plaintext);
It is strange to use StringPiece for input and std::string
for output in the Encrypt() and Decrypt() methods.

Can we not change these two methods?  Or can we also use
StringPiece for output?

This comment also applies to the private Crypt and CryptCTR
methods below.

http://codereview.chromium.org/7230037/diff/2003/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/7230037/diff/2003/crypto/encryptor_mac.cc#newc...
crypto/encryptor_mac.cc:38: iv_ = iv.as_string();
The as_string() method creates a temporary std::string object,
so we copy twice.

I think we should do
  iv.CopyToString(&iv_);

[Ryan Sleevi, 2011-06-28 22:56:12]

http://codereview.chromium.org/7230037/diff/2003/crypto/encryptor.h
File crypto/encryptor.h (right):

http://codereview.chromium.org/7230037/diff/2003/crypto/encryptor.h#newcode71
crypto/encryptor.h:71: bool Decrypt(const base::StringPiece& ciphertext,
std::string* plaintext);
On 2011/06/28 22:42:09, wtc wrote:
> It is strange to use StringPiece for input and std::string
> for output in the Encrypt() and Decrypt() methods.
> 
> Can we not change these two methods?  Or can we also use
> StringPiece for output?
> 
> This comment also applies to the private Crypt and CryptCTR
> methods below.

No. base::StringPiece doesn't carry with it any ownership semantics - it's just
a pointer+size holder - the original object needs to stick around for the
duration of the StringPiece.

As a result, it's not possible to transfer ownership of a new pointer to the
caller. This would be equivalent to the signature
Encrypt(T* input_pointer, scoped_refptr<T>* output_pointer)

Unfortunately, we can't do something like StringPiece* because whatever the
StringPiece pointed to would be out of scope when the function returned.

The semantics could be changed if the caller was guaranteed to give a properly
sized StringPiece, we could write into it, but I think that's leaky on several
levels - most importantly, being that it requires the callers to know about
padding and block sizes, which I think would be a poor API.

http://codereview.chromium.org/7230037/diff/2003/crypto/encryptor_mac.cc
File crypto/encryptor_mac.cc (right):

http://codereview.chromium.org/7230037/diff/2003/crypto/encryptor_mac.cc#newc...
crypto/encryptor_mac.cc:38: iv_ = iv.as_string();
On 2011/06/28 22:42:09, wtc wrote:
> The as_string() method creates a temporary std::string object,
> so we copy twice.
> 
> I think we should do
>   iv.CopyToString(&iv_);

You're right - I forgot about this convenience method.

[Ryan Sleevi, 2011-07-01 05:53:37]

ping?

[wtc, 2011-07-07 00:40:43]

LGTM.

http://codereview.chromium.org/7230037/diff/3007/crypto/encryptor_openssl.cc
File crypto/encryptor_openssl.cc (right):

http://codereview.chromium.org/7230037/diff/3007/crypto/encryptor_openssl.cc#...
crypto/encryptor_openssl.cc:70: iv_ = iv;
This should also be changed to
  iv.CopyToString(&iv_);

[commit-bot: I haz the power, 2011-07-08 02:53:11]

Change committed as 91800