X509NameAttribute::ValueAsString: Decode TeletexString as Latin1.

X509NameAttribute::ValueAsString: Decode TeletexString as Latin1.

BUG=717905, 715969
Review-Url: https://codereview.chromium.org/2865603002
Cr-Commit-Position: refs/heads/master@{#469812}
Committed: https://chromium.googlesource.com/chromium/src/+/788812f7788c07aa58487523ec70f9e921d78543

[mattm, 2017-05-05 02:34:50]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 02:35:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-05 02:38:08]

mattm@chromium.org changed reviewers:
+ eroman@chromium.org

[mattm, 2017-05-05 02:38:09]

I verified that all the platform native impls just treat them as latin1 or
cp1252(which is basically a superset of latin1, replacing some control codes
with printable chars).

[mattm, 2017-05-05 02:45:07]

On 2017/05/05 02:38:09, mattm wrote:
> I verified that all the platform native impls just treat them as latin1 or
> cp1252(which is basically a superset of latin1, replacing some control codes
> with printable chars).

Oh, and I only updated ValueAsString to keep the patch small and simple for
merging. 

ValueAsStringUnsafe is only used by 2 things now,
X509NameAttribute::AsRFC2253String and NormalizeValue(in
net/cert/internal/verify_name_match.cc).

NormalizeValue doesn't care, since it just does byte comparisons of
TeletexStrings, it doesn't actually call ValueAsStringUnsafe for TeletexStrings.
Though I guess if we've decided to treat them as Latin1 we could consider
relaxing that. Meh.

We may want to consider changing X509NameAttribute::AsRFC2253String to give more
friendly output for such TeletexStrings though.

[commit-bot: I haz the power, 2017-05-05 02:48:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 02:48:49]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mattm, 2017-05-05 04:12:31]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 04:12:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-05 04:19:14]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 04:19:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 05:31:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-05 05:31:53]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-05-05 17:55:29]

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
File net/cert/internal/parse_name.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_name.cc:202: // For nacl, just fall through to treating
like IA5String (ascii).
Why the difference for  nacl?

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:277:
ASSERT_NE(static_cast<X509Certificate*>(NULL), cert.get());
nit: nullptr instead of NULL, or ASSERT_TRUE() ?

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:317: //   (but with
use_platform_icu_alternatives it's not.)
What is the role of platform_icu_alternatives in this?

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:347: // "あ". (Probably. Not verified
against a real implementation.)
I haven't seen non-ascii in our source files before... is it safe to assume
utf8-encoding, or would it be better to write this as U+3042 ?

https://codereview.chromium.org/2865603002/diff/40001/net/test/test_data_dire...
File net/test/test_data_directory.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/test/test_data_dire...
net/test/test_data_directory.cc:30: base::FilePath GetTestCertsDirectory() {
Should this be re-implemented in terms of GetTestNetDataDirectory() ?

[mattm, 2017-05-05 19:56:09]

Patchset #4 (id:60001) has been deleted

[mattm, 2017-05-05 19:56:46]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 19:57:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-05 19:58:39]

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
File net/cert/internal/parse_name.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_name.cc:202: // For nacl, just fall through to treating
like IA5String (ascii).
On 2017/05/05 17:55:29, eroman wrote:
> Why the difference for  nacl?

the net/ build for nacl doesn't include base/net_string_util_icu.cc or the
//base:i18n and //third_party/icu deps. It looks like nacl actually does depend
on ICU through other chains, but simply adding the deps in net and using the
ConvertToUtf8 increased newlib_pnacl/remoting_client_plugin_newlib.pexe by 100KB
(release build).

Given David's comment on https://codereview.chromium.org/2861813002/ I don't
think nacl will care about this. So I don't really like making this difference,
but I was thinking it's probably not worth increasing nacl build size by 100KB.

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:277:
ASSERT_NE(static_cast<X509Certificate*>(NULL), cert.get());
On 2017/05/05 17:55:29, eroman wrote:
> nit: nullptr instead of NULL, or ASSERT_TRUE() ?

Done.

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:317: //   (but with
use_platform_icu_alternatives it's not.)
On 2017/05/05 17:55:29, eroman wrote:
> What is the role of platform_icu_alternatives in this?

net/base/net_string_util.h provides some wrapper funcs that on normal builds use
ICU, but on cronet use platform functions in order to reduce binary size. (The
iOS ones actually aren't implemented, but we haven't enabled use_byte_certs on
iOS yet so I'll worry about that later.)

So on android without platform_icu_alternatives, ConvertToUtf8(...,
kCharsetLatin1, ...) will use ICU which actually decodes as CP1252.
With platform_icu_alternatives it will decode as Latin1.

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:347: // "あ". (Probably. Not verified
against a real implementation.)
On 2017/05/05 17:55:29, eroman wrote:
> I haven't seen non-ascii in our source files before... is it safe to assume
> utf8-encoding, or would it be better to write this as U+3042 ?

Yeah, the chromium style guide says "Use UTF-8 file encodings", and the google
style guide says non-ascii characters are okay (for non-UI related strings,
since UI strings should not be hardcoded in any case):
http://google.github.io/styleguide/cppguide.html#Non-ASCII_Characters

https://codereview.chromium.org/2865603002/diff/40001/net/test/test_data_dire...
File net/test/test_data_directory.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/test/test_data_dire...
net/test/test_data_directory.cc:30: base::FilePath GetTestCertsDirectory() {
On 2017/05/05 17:55:29, eroman wrote:
> Should this be re-implemented in terms of GetTestNetDataDirectory() ?

Done.

[eroman, 2017-05-05 20:07:08]

lgtm

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
File net/cert/internal/parse_name.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_name.cc:202: // For nacl, just fall through to treating
like IA5String (ascii).
On 2017/05/05 19:58:39, mattm wrote:
> On 2017/05/05 17:55:29, eroman wrote:
> > Why the difference for  nacl?
> 
> the net/ build for nacl doesn't include base/net_string_util_icu.cc or the
> //base:i18n and //third_party/icu deps. It looks like nacl actually does
depend
> on ICU through other chains, but simply adding the deps in net and using the
> ConvertToUtf8 increased newlib_pnacl/remoting_client_plugin_newlib.pexe by
100KB
> (release build).
> 
> Given David's comment on https://codereview.chromium.org/2861813002/ I don't
> think nacl will care about this. So I don't really like making this
difference,
> but I was thinking it's probably not worth increasing nacl build size by
100KB.

It is unfortunate to have a different behavior here, should it ever grow more
dependencies on this.

Can you add a comment explaining this, and perhaps a TODO as well?

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/cert/x509_certifica...
net/cert/x509_certificate_unittest.cc:347: // "あ". (Probably. Not verified
against a real implementation.)
On 2017/05/05 19:58:39, mattm wrote:
> On 2017/05/05 17:55:29, eroman wrote:
> > I haven't seen non-ascii in our source files before... is it safe to assume
> > utf8-encoding, or would it be better to write this as U+3042 ?
> 
> Yeah, the chromium style guide says "Use UTF-8 file encodings", and the google
> style guide says non-ascii characters are okay (for non-UI related strings,
> since UI strings should not be hardcoded in any case):
> http://google.github.io/styleguide/cppguide.html#Non-ASCII_Characters

Thanks!

[mattm, 2017-05-05 20:16:25]

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
File net/cert/internal/parse_name.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_name.cc:202: // For nacl, just fall through to treating
like IA5String (ascii).
On 2017/05/05 20:07:07, eroman wrote:
> On 2017/05/05 19:58:39, mattm wrote:
> > On 2017/05/05 17:55:29, eroman wrote:
> > > Why the difference for  nacl?
> > 
> > the net/ build for nacl doesn't include base/net_string_util_icu.cc or the
> > //base:i18n and //third_party/icu deps. It looks like nacl actually does
> depend
> > on ICU through other chains, but simply adding the deps in net and using the
> > ConvertToUtf8 increased newlib_pnacl/remoting_client_plugin_newlib.pexe by
> 100KB
> > (release build).
> > 
> > Given David's comment on https://codereview.chromium.org/2861813002/ I don't
> > think nacl will care about this. So I don't really like making this
> difference,
> > but I was thinking it's probably not worth increasing nacl build size by
> 100KB.
> 
> It is unfortunate to have a different behavior here, should it ever grow more
> dependencies on this.
> 
> Can you add a comment explaining this, and perhaps a TODO as well?

Done.

[mattm, 2017-05-05 20:17:25]

The CQ bit was checked by mattm@chromium.org

[mattm, 2017-05-05 20:17:26]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2865603002/#ps100001
(title: "add comment/todo about nacl difference")

[commit-bot: I haz the power, 2017-05-05 20:18:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 23:49:12]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494015445906650,
"parent_rev": "b467e7c5b7629f8182b569e2335cca84e4fc8bb9", "commit_rev":
"788812f7788c07aa58487523ec70f9e921d78543"}

[commit-bot: I haz the power, 2017-05-05 23:49:22]

Description was changed from

==========
X509NameAttribute::ValueAsString: Decode TeletexString as Latin1.

BUG=717905,715969
==========

to

==========
X509NameAttribute::ValueAsString: Decode TeletexString as Latin1.

BUG=717905,715969

Review-Url: https://codereview.chromium.org/2865603002
Cr-Commit-Position: refs/heads/master@{#469812}
Committed:
https://chromium.googlesource.com/chromium/src/+/788812f7788c07aa58487523ec70...
==========

[commit-bot: I haz the power, 2017-05-05 23:49:23]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/788812f7788c07aa58487523ec70...

[davidben, 2017-05-12 16:47:25]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2017-05-12 16:47:26]

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
File net/cert/internal/parse_name.cc (right):

https://codereview.chromium.org/2865603002/diff/40001/net/cert/internal/parse...
net/cert/internal/parse_name.cc:202: // For nacl, just fall through to treating
like IA5String (ascii).
On 2017/05/05 20:16:25, mattm wrote:
> On 2017/05/05 20:07:07, eroman wrote:
> > On 2017/05/05 19:58:39, mattm wrote:
> > > On 2017/05/05 17:55:29, eroman wrote:
> > > > Why the difference for  nacl?
> > > 
> > > the net/ build for nacl doesn't include base/net_string_util_icu.cc or the
> > > //base:i18n and //third_party/icu deps. It looks like nacl actually does
> > depend
> > > on ICU through other chains, but simply adding the deps in net and using
the
> > > ConvertToUtf8 increased newlib_pnacl/remoting_client_plugin_newlib.pexe by
> > 100KB
> > > (release build).
> > > 
> > > Given David's comment on https://codereview.chromium.org/2861813002/ I
don't
> > > think nacl will care about this. So I don't really like making this
> > difference,
> > > but I was thinking it's probably not worth increasing nacl build size by
> > 100KB.
> > 
> > It is unfortunate to have a different behavior here, should it ever grow
more
> > dependencies on this.
> > 
> > Can you add a comment explaining this, and perhaps a TODO as well?
> 
> Done.

FWIW, Latin1 doesn't need much code to convert to UTF-8:

out->clear();
size_t len = value.Length();
for (char c : value.AsStringPiece()) {
  if (static_cast<uint8_t>(c) > 0x7f)
    len++;
}
out->reserve(len);
for (char c : value.AsStringPiece()) {
  uint8_t u = static_cast<uint8_t>(c);
  if (u > 0x7f) {
    out->push_back(0xc0 | (u >> 6));
    out->push_back(0x80 | (u & 0x3f));
  } else {
    out->push_back(u);
  }
}
DCHECK_EQ(len, out->size());