Remove dangerous std::string constructor for der::Input

Remove dangerous std::string constructor for der::Input

BUG=490472
Committed: https://crrev.com/ab7f492e15f167aace4d448b8e00053788038760
Cr-Commit-Position: refs/heads/master@{#333201}

[nharper, 2015-05-26 15:39:48]

nharper@chromium.org changed reviewers:
+ mattm@chromium.org, rsleevi@chromium.org

[nharper, 2015-05-26 17:02:03]

Fix null-terminated strings bug, and use a static factory method instead of
another constructor

[Ryan Sleevi, 2015-05-26 18:53:30]

I'd prefer this be addressed simply be removing the constructor and updating the
tests.

https://codereview.chromium.org/1160643002/diff/20001/net/der/input.h
File net/der/input.h (right):

https://codereview.chromium.org/1160643002/diff/20001/net/der/input.h#newcode56
net/der/input.h:56: }
Strictly on a STYLE basis, this shouldn't be interleaved between constructors.

However, from this CL, it doesn't seem that this is actually needed?

https://codereview.chromium.org/1160643002/diff/20001/net/der/input_unittest.cc
File net/der/input_unittest.cc (right):

https://codereview.chromium.org/1160643002/diff/20001/net/der/input_unittest....
net/der/input_unittest.cc:21: memcpy(input_copy, kInput, arraysize(kInput));
It's unclear what this is testing or why it's necessary

[nharper, 2015-05-26 19:22:10]

On 2015/05/26 18:53:30, Ryan Sleevi wrote:
> I'd prefer this be addressed simply be removing the constructor and updating
the
> tests.
I can do that. Just to be clear, that will be changing something like:

der::Input("hello")

to:

der::Input({'h', 'e', 'l', 'l', 'o'})

I think I'd need to have some method like FromCString (or the constructor in
ps1) if I want to take a c/c++ raw string literal instead of an unsigned char
array.

> 
> https://codereview.chromium.org/1160643002/diff/20001/net/der/input.h
> File net/der/input.h (right):
> 
>
https://codereview.chromium.org/1160643002/diff/20001/net/der/input.h#newcode56
> net/der/input.h:56: }
> Strictly on a STYLE basis, this shouldn't be interleaved between constructors.
> 
> However, from this CL, it doesn't seem that this is actually needed?
> 
>
https://codereview.chromium.org/1160643002/diff/20001/net/der/input_unittest.cc
> File net/der/input_unittest.cc (right):
> 
>
https://codereview.chromium.org/1160643002/diff/20001/net/der/input_unittest....
> net/der/input_unittest.cc:21: memcpy(input_copy, kInput, arraysize(kInput));
> It's unclear what this is testing or why it's necessary

[Ryan Sleevi, 2015-05-26 19:35:54]

On 2015/05/26 19:22:10, nharper wrote:
> I can do that. Just to be clear, that will be changing something like:
> 
> der::Input("hello")
> 
> to:
> 
> der::Input({'h', 'e', 'l', 'l', 'o'})

Not quite; braced initializer syntax is still not allowed (and the above example
demonstrates certain degrees of ambiguity with respect to implicit initializers)

const uint8_t date_1[] = {'h', 'e', 'l', 'l', 'o'};
der::Input input(date_1);

[nharper, 2015-05-26 22:09:34]

Remove FromCString; use uint8_t array literals instead of string literals now

[nharper, 2015-05-26 22:11:49]

https://codereview.chromium.org/1160643002/diff/20001/net/der/input_unittest.cc
File net/der/input_unittest.cc (right):

https://codereview.chromium.org/1160643002/diff/20001/net/der/input_unittest....
net/der/input_unittest.cc:21: memcpy(input_copy, kInput, arraysize(kInput));
On 2015/05/26 18:53:30, Ryan Sleevi wrote:
> It's unclear what this is testing or why it's necessary

It's testing that 2 Inputs are equal that point to different data, but the
underlying data is the same. The above test (with test and test2) isn't enough
because they both point to the same underlying data kInput.

https://codereview.chromium.org/1160643002/diff/40001/net/der/parse_values_un...
File net/der/parse_values_unittest.cc (right):

https://codereview.chromium.org/1160643002/diff/40001/net/der/parse_values_un...
net/der/parse_values_unittest.cc:100: 'Z'};
I don't particularly like how these look. I tried putting it all on one line
(like the literal for utc_time_relaxed_too_long on line 80), but it is too long
to fit on one line with the proper indentation.

[Ryan Sleevi, 2015-06-03 23:09:11]

OK, after our chat, I think probably the least-gross way is probably to put a
test helper in the unit test with the from string, something like

namespace {

template <size_t N>
der::Input FromStringConstant(const char(&data)[N]) {
  return der::Input(static_cast<const uint8_t*>(data), N);
}

}  // namespace

https://codereview.chromium.org/1160643002/diff/40001/net/der/parse_values_un...
File net/der/parse_values_unittest.cc (right):

https://codereview.chromium.org/1160643002/diff/40001/net/der/parse_values_un...
net/der/parse_values_unittest.cc:100: 'Z'};
On 2015/05/26 22:11:49, nharper wrote:
> I don't particularly like how these look. I tried putting it all on one line
> (like the literal for utc_time_relaxed_too_long on line 80), but it is too
long
> to fit on one line with the proper indentation.

As soon as the new clang rolls in to Chromium, this should be fixed. I think.

[nharper, 2015-06-05 23:26:52]

rebase

[nharper, 2015-06-06 00:05:30]

Use helper in test for constructing Input from string literal

[nharper, 2015-06-06 00:07:59]

Put helper in anonymous namespace

[nharper, 2015-06-06 00:10:35]

Added helper function to parse_values_unittest to make it more readable.

[Ryan Sleevi, 2015-06-06 00:16:34]

lgtm

[nharper, 2015-06-06 00:17:50]

The CQ bit was checked by nharper@chromium.org

[commit-bot: I haz the power, 2015-06-06 00:18:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1160643002/90001

[commit-bot: I haz the power, 2015-06-06 01:47:29]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-06-06 01:48:16]

Patchset 6 (id:??) landed as
https://crrev.com/ab7f492e15f167aace4d448b8e00053788038760
Cr-Commit-Position: refs/heads/master@{#333201}