Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8429034:
Upstream: Build net_unittests for Android. (Closed)
|
Created:
9 years, 1 month ago by Jing Zhao Modified:
9 years ago Reviewers:
willchan no longer on Chromium, Ryan Sleevi, michaelbai, John Grabowski, joth, wtc, jingzhao, Mark Mentovai, bulach, mmenke CC:
chromium-reviews, cbentzel+watch_chromium.org, Paweł Hajdan Jr., darin-cc_chromium.org, mmenke, brettw-cc_chromium.org Visibility:
Public. |
DescriptionUpstream: Build net_unittests for Android.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110902
Patch Set 1 #
Total comments: 26
Patch Set 2 : address comments #
Total comments: 30
Patch Set 3 : address comments #Patch Set 4 : fix llog -- it's not for all targets #
Total comments: 2
Patch Set 5 : fix a bug in ssl_client_socket_openssl.cc and address comments #
Total comments: 20
Patch Set 6 : address comments in ssl_client_socket_openssl.cc #Patch Set 7 : address comments #Patch Set 8 : style fix #Patch Set 9 : fix build #
Total comments: 3
Patch Set 10 : update common.gypi #Patch Set 11 : update crypto.gyp #
Total comments: 1
Patch Set 12 : split out unreviewed files #
Messages
Total messages: 27 (0 generated)
http://codereview.chromium.org/8429034/diff/1/base/base.gypi File base/base.gypi (right): http://codereview.chromium.org/8429034/diff/1/base/base.gypi#newcode609 base/base.gypi:609: ['exclude', '^debug/stack_trace_posix.cc'], It seemed that this 'reg' way is not preferred, you may use 'sources!' see http://codereview.chromium.org/8387053/ http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... File net/base/x509_certificate_openssl.cc (left): http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... net/base/x509_certificate_openssl.cc:29: namespace { Why did you remove the this? http://codereview.chromium.org/8429034/diff/1/net/socket/ssl_client_socket_op... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/1/net/socket/ssl_client_socket_op... net/socket/ssl_client_socket_openssl.cc:810: } It seemed that this code is not android specific, should it be upstreamed?
Excellent to see progress on this front http://codereview.chromium.org/8429034/diff/1/base/base.gypi File base/base.gypi (right): http://codereview.chromium.org/8429034/diff/1/base/base.gypi#newcode440 base/base.gypi:440: ['exclude', '^files/file_path_watcher_kqueue.cc'], I landed a fix for this today; see http://codereview.chromium.org/8387053/ http://codereview.chromium.org/8429034/diff/1/build/all_android.gyp File build/all_android.gyp (right): http://codereview.chromium.org/8429034/diff/1/build/all_android.gyp#newcode27 build/all_android.gyp:27: '../net/net.gyp:net_unittests', :-) http://codereview.chromium.org/8429034/diff/1/build/android/system.gyp File build/android/system.gyp (right): http://codereview.chromium.org/8429034/diff/1/build/android/system.gyp#newcode1 build/android/system.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. Comment on the origin of this file (e.g. likely build/linux/system.gyp) http://codereview.chromium.org/8429034/diff/1/crypto/crypto.gyp File crypto/crypto.gyp (right): http://codereview.chromium.org/8429034/diff/1/crypto/crypto.gyp#newcode46 crypto/crypto.gyp:46: [ 'OS == "android"', { Hmm looks like this condition above might get hit: [ 'os_posix == 1 and OS != "mac"', { Can you confirm it does not? http://codereview.chromium.org/8429034/diff/1/net/base/platform_mime_util_lin... File net/base/platform_mime_util_linux.cc (right): http://codereview.chromium.org/8429034/diff/1/net/base/platform_mime_util_lin... net/base/platform_mime_util_linux.cc:24: // "android::GetMimeTypeFromExtension(ext, result)" once we support JNI. Please start spreadsheet docing these "once we have JNI" issues http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... File net/base/x509_certificate_openssl_android.cc (right): http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... net/base/x509_certificate_openssl_android.cc:15: struct DERCache { Hmm where do other platforms pick this up? http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... net/base/x509_certificate_openssl_android.cc:34: // TODO(jingzhao): Uncomment the following code once we support JNI. ditto on doc http://codereview.chromium.org/8429034/diff/1/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8429034/diff/1/net/net.gyp#newcode954 net/net.gyp:954: ['include', '^base/platform_mime_util_linux.cc'], '^base/platform_mime_util_linux\\.cc$' http://codereview.chromium.org/8429034/diff/1/net/net.gyp#newcode1247 net/net.gyp:1247: 'sources/': [ 'sources!': [ .. then no regexp ]
http://codereview.chromium.org/8429034/diff/1/build/android/system.gyp File build/android/system.gyp (right): http://codereview.chromium.org/8429034/diff/1/build/android/system.gyp#newcode1 build/android/system.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. Random drive by nit: Update copyright year. One of the other files you're modifying says 2010, too. (Obviously, don't wait for me to sign off on the patch)
Thanks for your review! Could you take another look? http://codereview.chromium.org/8429034/diff/1/base/base.gypi File base/base.gypi (right): http://codereview.chromium.org/8429034/diff/1/base/base.gypi#newcode440 base/base.gypi:440: ['exclude', '^files/file_path_watcher_kqueue.cc'], On 2011/11/01 23:09:34, John Grabowski wrote: > I landed a fix for this today; see http://codereview.chromium.org/8387053/ Removed. http://codereview.chromium.org/8429034/diff/1/base/base.gypi#newcode609 base/base.gypi:609: ['exclude', '^debug/stack_trace_posix.cc'], On 2011/11/01 16:37:59, michaelbai wrote: > It seemed that this 'reg' way is not preferred, you may use 'sources!' > > see http://codereview.chromium.org/8387053/ Done. http://codereview.chromium.org/8429034/diff/1/build/android/system.gyp File build/android/system.gyp (right): http://codereview.chromium.org/8429034/diff/1/build/android/system.gyp#newcode1 build/android/system.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2011/11/01 23:15:22, Matt Menke wrote: > Random drive by nit: Update copyright year. One of the other files you're > modifying says 2010, too. (Obviously, don't wait for me to sign off on the > patch) Done. http://codereview.chromium.org/8429034/diff/1/build/android/system.gyp#newcode1 build/android/system.gyp:1: # Copyright (c) 2010 The Chromium Authors. All rights reserved. On 2011/11/01 23:09:34, John Grabowski wrote: > Comment on the origin of this file (e.g. likely build/linux/system.gyp) It's newly written, because we use openssl while Linux uses nss. http://codereview.chromium.org/8429034/diff/1/crypto/crypto.gyp File crypto/crypto.gyp (right): http://codereview.chromium.org/8429034/diff/1/crypto/crypto.gyp#newcode46 crypto/crypto.gyp:46: [ 'OS == "android"', { On 2011/11/01 23:09:34, John Grabowski wrote: > Hmm looks like this condition above might get hit: > [ 'os_posix == 1 and OS != "mac"', { > > Can you confirm it does not? Fixed the condition above. Thanks for catching it. http://codereview.chromium.org/8429034/diff/1/net/base/platform_mime_util_lin... File net/base/platform_mime_util_linux.cc (right): http://codereview.chromium.org/8429034/diff/1/net/base/platform_mime_util_lin... net/base/platform_mime_util_linux.cc:24: // "android::GetMimeTypeFromExtension(ext, result)" once we support JNI. On 2011/11/01 23:09:34, John Grabowski wrote: > Please start spreadsheet docing these "once we have JNI" issues Done. http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... File net/base/x509_certificate_openssl.cc (left): http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... net/base/x509_certificate_openssl.cc:29: namespace { On 2011/11/01 16:37:59, michaelbai wrote: > Why did you remove the this? Methods in this anonymous namespace were only used in this file, but now we also use GetDERAndCacheIfNeeded in net/base/x509_certificate_openssl_android.cc. http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... File net/base/x509_certificate_openssl_android.cc (right): http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... net/base/x509_certificate_openssl_android.cc:15: struct DERCache { On 2011/11/01 23:09:34, John Grabowski wrote: > Hmm where do other platforms pick this up? It was only used in net/base/x509_certificate_openssl.cc. Should I move it to net/base/x509_certificate.h, like what I already did? http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... net/base/x509_certificate_openssl_android.cc:34: // TODO(jingzhao): Uncomment the following code once we support JNI. On 2011/11/01 23:09:34, John Grabowski wrote: > ditto on doc Done. http://codereview.chromium.org/8429034/diff/1/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8429034/diff/1/net/net.gyp#newcode954 net/net.gyp:954: ['include', '^base/platform_mime_util_linux.cc'], On 2011/11/01 23:09:34, John Grabowski wrote: > '^base/platform_mime_util_linux\\.cc$' Done. http://codereview.chromium.org/8429034/diff/1/net/net.gyp#newcode1247 net/net.gyp:1247: 'sources/': [ On 2011/11/01 23:09:34, John Grabowski wrote: > 'sources!': [ .. then no regexp ] Done. http://codereview.chromium.org/8429034/diff/1/net/socket/ssl_client_socket_op... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/1/net/socket/ssl_client_socket_op... net/socket/ssl_client_socket_openssl.cc:810: } On 2011/11/01 16:37:59, michaelbai wrote: > It seemed that this code is not android specific, should it be upstreamed? http://codereview.chromium.org/8156001/ This change changed next_protos from a string to a vector, so we need to update this file to build for Android. Should I separate this file into another changelist?
I'd like joth and marcus to review it
some comments... great to see this moving along. http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc File net/base/dns_reloader.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc#new... net/base/dns_reloader.cc:7: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD) && !defined(OS_ANDROID) uh! I wonder why this isn't using a .gyp rule to exclude? Anyway, please wrap to 80 col http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc#new... net/base/dns_reloader.cc:37: sounds like there should be an additional comment here about android (but I'm not totally sure what it would say!) http://codereview.chromium.org/8429034/diff/4003/net/base/dnsrr_resolver.cc File net/base/dnsrr_resolver.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/dnsrr_resolver.cc#n... net/base/dnsrr_resolver.cc:494: #endif // defined(OS_POSIX) && !defined(OS_ANDROID) surprised there's no #else here. (Windows never needs to ParseFromResponse? seems to be missing a comment at least ... ideally a default #error case) For ANDROID I think it should be a NOTIMPLEMENTED, anyway. http://codereview.chromium.org/8429034/diff/4003/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1088: #if defined(OS_POSIX) && !defined(OS_MACOSX) ISTM this #if clause should match the one below: if we set HOST_RESOLVER_LOOPBACK_ONLY here it could only be unset by the OnDNSChanged callback. so I wonder (a) why they area already different (bug in openbsd port?) and (b) if they can just be merged into one combined chunk. For bonus points, I'd love a quick comment here as a reminder why we only care about HOST_RESOLVER_LOOPBACK_ONLY on certain platforms.... :) My reading of host_resolver_proc.cc is it comes down to AI_ADDRCONFIG handling, and in that it should be identical to Linux hence we will want this at some point. http://codereview.chromium.org/8429034/diff/4003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1093: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD) && !defined(OS_ANDROID) nit: word wrap http://codereview.chromium.org/8429034/diff/4003/net/base/platform_mime_util_... File net/base/platform_mime_util_linux.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/platform_mime_util_... net/base/platform_mime_util_linux.cc:24: // "android::GetMimeTypeFromExtension(ext, result)" once we support JNI. is NOTIMPLEMENTED() too spammy? http://codereview.chromium.org/8429034/diff/4003/net/base/platform_mime_util_... net/base/platform_mime_util_linux.cc:36: // TODO(thestig): This is a temporary hack until we can fix this side note, the : is optional in http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#TODO_Comments (but agree it's more common to have them) http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate.h#... net/base/x509_certificate.h:565: DERCache* der_cache); this doesn't belong here, as it's only needed on the _openssl versions. I think we can just merge x509_certificate_openssl.cc and x509_certificate_openssl_android.cc now they are living in the same repo (with the appropriate internal #ifdefs) -or- if we want to keep them separete introduce a x509_certificate_openssl.h to declare the common functions between them. I prefer the former though, as it will make it easier to share common parts of VerifyInternal(). Note that either way, the GetChainDEREncodedBytes can be removed from this class above. That would either live in the new .h file, or just be an internal detail of the merged .cc I'm happy to do a patch for this if that's not very clear! http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... File net/base/x509_certificate_openssl.cc (left): http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... net/base/x509_certificate_openssl.cc:303: } // namespace shouldn't be any need for any of these edits http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... File net/base/x509_certificate_openssl_android.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... net/base/x509_certificate_openssl_android.cc:28: android::VerifyX509CertChain(cert_bytes, hostname, "RSA"); just #if 0 out this one line, and in the #else initialize |result| to android::VERIFY_INVOCATION_ERROR (and do a NOTIMPLEMENTED for good measure) http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... net/base/x509_certificate_openssl_android.cc:46: void X509Certificate::GetChainDEREncodedBytes( this method should just be removed from here and put intox509_certificate_openssl.cc (but see also my comment to just fold these two files together.) http://codereview.chromium.org/8429034/diff/4003/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8429034/diff/4003/net/net.gyp#newcode938 net/net.gyp:938: 'dns/dns_config_service_posix.cc', I see this service isn't actually used in chrome anywhere yet, and http://crbug.com/90881 doesn't seem to mention what it will be used for. Have you checked with them? Just wonder if we need a TODO or bug link here to track if/when we would need a corresponding implementation of it. http://codereview.chromium.org/8429034/diff/4003/net/net.gyp#newcode942 net/net.gyp:942: # missing JNI, add them back when JNI is ready. nit: missing JNI -> missing availability of JNI bindings in the build system, or missing availability to test code? http://codereview.chromium.org/8429034/diff/4003/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:805: break; where is this edit coming from? we should be using SSL_select_next_proto for this.
LGTM from me conditional on a Joth LGTM. Note you will need OWNERS LGTM before sending it in. http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... File net/base/x509_certificate_openssl_android.cc (right): http://codereview.chromium.org/8429034/diff/1/net/base/x509_certificate_opens... net/base/x509_certificate_openssl_android.cc:15: struct DERCache { On 2011/11/02 16:22:55, Jing Zhao wrote: > On 2011/11/01 23:09:34, John Grabowski wrote: > > Hmm where do other platforms pick this up? > > It was only used in net/base/x509_certificate_openssl.cc. Should I move it to > net/base/x509_certificate.h, like what I already did? Per subsequent follow-up, follow joth's instructions.
Joth, thanks a lot for your review. Please take another look. http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc File net/base/dns_reloader.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc#new... net/base/dns_reloader.cc:7: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD) && !defined(OS_ANDROID) On 2011/11/02 18:57:05, joth wrote: > uh! I wonder why this isn't using a .gyp rule to exclude? > > Anyway, please wrap to 80 col Done. http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc#new... net/base/dns_reloader.cc:37: On 2011/11/02 18:57:05, joth wrote: > sounds like there should be an additional comment here about android (but I'm > not totally sure what it would say!) > I don't know either.. What I know is Android doesn't have res_ninit/res_nclose. Do you know someone familiar with this? http://codereview.chromium.org/8429034/diff/4003/net/base/dnsrr_resolver.cc File net/base/dnsrr_resolver.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/dnsrr_resolver.cc#n... net/base/dnsrr_resolver.cc:494: #endif // defined(OS_POSIX) && !defined(OS_ANDROID) On 2011/11/02 18:57:05, joth wrote: > surprised there's no #else here. (Windows never needs to ParseFromResponse? > seems to be missing a comment at least ... ideally a default #error case) > > For ANDROID I think it should be a NOTIMPLEMENTED, anyway. ParseFromResponse() is only called by Do() in the previous OS_POSIX block. The OS_WIN block doesn't use it. In net/base/dnsrr_resolver.h, the comment of this function says "For testing only". How about put the entire function between #if and #endif, since other platforms never use it? http://codereview.chromium.org/8429034/diff/4003/net/base/host_resolver_impl.cc File net/base/host_resolver_impl.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/host_resolver_impl.... net/base/host_resolver_impl.cc:1093: #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD) && !defined(OS_ANDROID) On 2011/11/02 18:57:05, joth wrote: > nit: word wrap Just found I only want to skip EnsureDnsReloaderInit() for Android. http://codereview.chromium.org/8429034/diff/4003/net/base/platform_mime_util_... File net/base/platform_mime_util_linux.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/platform_mime_util_... net/base/platform_mime_util_linux.cc:24: // "android::GetMimeTypeFromExtension(ext, result)" once we support JNI. On 2011/11/02 18:57:05, joth wrote: > is NOTIMPLEMENTED() too spammy? Does it look better now? http://codereview.chromium.org/8429034/diff/4003/net/base/platform_mime_util_... net/base/platform_mime_util_linux.cc:36: // TODO(thestig): This is a temporary hack until we can fix this On 2011/11/02 18:57:05, joth wrote: > side note, the : is optional in > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#TODO_Comments > (but agree it's more common to have them) :) http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate.h#... net/base/x509_certificate.h:565: DERCache* der_cache); On 2011/11/02 18:57:05, joth wrote: > this doesn't belong here, as it's only needed on the _openssl versions. > > I think we can just merge x509_certificate_openssl.cc and > x509_certificate_openssl_android.cc now they are living in the same repo (with > the appropriate internal #ifdefs) > > -or- if we want to keep them separete introduce a x509_certificate_openssl.h to > declare the common functions between them. > > I prefer the former though, as it will make it easier to share common parts of > VerifyInternal(). > > > Note that either way, the GetChainDEREncodedBytes can be removed from this class > above. That would either live in the new .h file, or just be an internal detail > of the merged .cc > > I'm happy to do a patch for this if that's not very clear! Thanks. Merged as you suggested. http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... File net/base/x509_certificate_openssl.cc (left): http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... net/base/x509_certificate_openssl.cc:303: } // namespace On 2011/11/02 18:57:05, joth wrote: > shouldn't be any need for any of these edits Done. http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... File net/base/x509_certificate_openssl_android.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... net/base/x509_certificate_openssl_android.cc:28: android::VerifyX509CertChain(cert_bytes, hostname, "RSA"); On 2011/11/02 18:57:05, joth wrote: > just #if 0 out this one line, and in the #else initialize |result| to > android::VERIFY_INVOCATION_ERROR > (and do a NOTIMPLEMENTED for good measure) Done. http://codereview.chromium.org/8429034/diff/4003/net/base/x509_certificate_op... net/base/x509_certificate_openssl_android.cc:46: void X509Certificate::GetChainDEREncodedBytes( On 2011/11/02 18:57:05, joth wrote: > this method should just be removed from here and put > intox509_certificate_openssl.cc > > (but see also my comment to just fold these two files together.) Done. http://codereview.chromium.org/8429034/diff/4003/net/net.gyp File net/net.gyp (right): http://codereview.chromium.org/8429034/diff/4003/net/net.gyp#newcode938 net/net.gyp:938: 'dns/dns_config_service_posix.cc', On 2011/11/02 18:57:05, joth wrote: > I see this service isn't actually used in chrome anywhere yet, and > http://crbug.com/90881 doesn't seem to mention what it will be used for. Have > you checked with them? Just wonder if we need a TODO or bug link here to track > if/when we would need a corresponding implementation of it. Added it back with a few #if clauses. http://codereview.chromium.org/8429034/diff/4003/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:805: break; On 2011/11/02 18:57:05, joth wrote: > where is this edit coming from? we should be using SSL_select_next_proto for > this. http://codereview.chromium.org/8156001/ This change changed next_protos from a string to a vector, but SSL_select_next_proto takes a string as its fifth parameter, so I had the logic here.
Excellent! LGTM with a couple small suggestions. http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc File net/base/dns_reloader.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc#new... net/base/dns_reloader.cc:37: On 2011/11/03 17:49:08, Jing Zhao wrote: > On 2011/11/02 18:57:05, joth wrote: > > sounds like there should be an additional comment here about android (but I'm > > not totally sure what it would say!) > > > > I don't know either.. What I know is Android doesn't have res_ninit/res_nclose. > Do you know someone familiar with this? actually, looking at it again, I think it's pretty simple: // Android does not have /etc/resolv.conf at all the system takes care of nameserver changes, so none of this is not needed. (This is not strictly OS_POSIX at all) http://codereview.chromium.org/8429034/diff/4003/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:805: break; On 2011/11/03 17:49:08, Jing Zhao wrote: > On 2011/11/02 18:57:05, joth wrote: > > where is this edit coming from? we should be using SSL_select_next_proto for > > this. > > http://codereview.chromium.org/8156001/ > This change changed next_protos from a string to a vector, but > SSL_select_next_proto takes a string as its fifth parameter, so I had the logic > here. ouch! that's double bad. first, linux_redux didn't get a build break because the gnu vector has a non-standard data() member the reinterpret_cast means it still builds (all be it, doing totally the wrong thing). And second, we obviously don't have any test coverage for this code path :-( Thanks for finding & fixing. Did you manage to test this somehow? http://codereview.chromium.org/8429034/diff/12002/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/8429034/diff/12002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:35: success_ = false; I like this tidy up in terms of un-interleaving the #if and normal ifs, however this comment doesn't seem to make sense up here - maybe move down into one of the blocks below (next to the if() also, I'd be tempted to put an outer #if !defined(OS_ANDROID) around the entire rest of the method just to make it clear the success_ = false is the only bit we need. (And maybe a NOTIMPLEMENTED for the ANDROID case)
Joth, thanks a lot. Please take another look. http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc File net/base/dns_reloader.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/base/dns_reloader.cc#new... net/base/dns_reloader.cc:37: On 2011/11/03 19:45:02, joth wrote: > On 2011/11/03 17:49:08, Jing Zhao wrote: > > On 2011/11/02 18:57:05, joth wrote: > > > sounds like there should be an additional comment here about android (but > I'm > > > not totally sure what it would say!) > > > > > > > I don't know either.. What I know is Android doesn't have > res_ninit/res_nclose. > > Do you know someone familiar with this? > > actually, looking at it again, I think it's pretty simple: > // Android does not have /etc/resolv.conf at all the system takes care of > nameserver changes, so none of this is not needed. > (This is not strictly OS_POSIX at all) Added. Thanks for helping me out. http://codereview.chromium.org/8429034/diff/4003/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/4003/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:805: break; On 2011/11/03 19:45:02, joth wrote: > On 2011/11/03 17:49:08, Jing Zhao wrote: > > On 2011/11/02 18:57:05, joth wrote: > > > where is this edit coming from? we should be using SSL_select_next_proto for > > > this. > > > > http://codereview.chromium.org/8156001/ > > This change changed next_protos from a string to a vector, but > > SSL_select_next_proto takes a string as its fifth parameter, so I had the > logic > > here. > > ouch! that's double bad. first, linux_redux didn't get a build break because the > gnu vector has a non-standard data() member the reinterpret_cast means it still > builds (all be it, doing totally the wrong thing). And second, we obviously > don't have any test coverage for this code path :-( > > Thanks for finding & fixing. > > Did you manage to test this somehow? No I didn't find a way to test this. I fixed two bugs in my original code, and now it should be correct.. http://codereview.chromium.org/8429034/diff/12002/net/dns/dns_config_service_... File net/dns/dns_config_service_posix.cc (right): http://codereview.chromium.org/8429034/diff/12002/net/dns/dns_config_service_... net/dns/dns_config_service_posix.cc:35: success_ = false; On 2011/11/03 19:45:02, joth wrote: > I like this tidy up in terms of un-interleaving the #if and normal ifs, however > this comment doesn't seem to make sense up here - maybe move down into one of > the blocks below (next to the if() > > also, I'd be tempted to put an outer #if !defined(OS_ANDROID) around the entire > rest of the method just to make it clear the success_ = false is the only bit we > need. > (And maybe a NOTIMPLEMENTED for the ANDROID case) Done.
http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... File net/base/x509_certificate_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:542: return OK; random drive by: What does it mean if android::VerifyX509CertChain returns VERIFY_OK, but the VerifyNameMatch (on line 525/526) fails? Right now, that error would be suppressed. As/if/when Chrom[e/ium] implements more stringent checks, this could potentially be an issue. One lesser note (perhaps a TODO, or perhaps never for the android impl.) is to have the android::VerifyX509CertChain return the constructed certification path. The reason for this is purely presentation (being able to display the certificate chain). It'd be a low priority, as the display code is not yet wired up, but some of the unit tests are testing that unrelated, untrusted intermediates are pruned from the certification path. X509Certificate::Verify() has a default implementation setting the verified_chain = this, but the expectation is that VerifyInternal() will update the chain as/when appropriate. http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:667: OSCertHandles cert_handles(intermediate_ca_certs_); Two things: 1) Why not implement this in terms of GetDEREncoded (line 612), to avoid diverging DER-accessing implementations? Since you end up copying |der_cache.data| to a string anyways, there is no extra copy overhead here. http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:671: cert_handles.insert(cert_handles.begin(), cert_handle_); 2) This isn't correct how X509Certificate stores its chain, so I'm surprised this works. |intermediate_ca_certs_| contains only the intermediates supplied by the server - it never contains the end-entity cert (|cert_handle_|). For servers who supply a certificate chain > 1 (ie: most?), this would fail to consider the EE certificate. http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:802: *out = (unsigned char *) in + 1; BUG? Without checking the implementation of SSL_select_next_proto, what happens if |in| or |inlen| are small (hostile? incorrect?) values. Paranoid example, |inlen| == 1, but |*in| == 5. That means line 820 will attempt to read an invalid range. Is this input already validated for correctness? Should/can you document this? style nit: Chromium/Google open-source style uses C++ casts, as convenient as the C-style cast may be http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting *out = reinterpret_cast<unsigned char*>(in) + i + 1; This is consistent with say, lines 795 and 820. http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:807: j != ssl_config_.next_protos.end(); j++) { nit: ++j http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:809: memcmp(&in[i + 1], j->data(), in[i]) == 0) { I'm a little nervous, quite possibly unreasonably so, when I see a straight up memcmp in crypto/security code. While my gut says this probably has no security relevance (and is thus OK), can you double check with the implementation (say, SSL_select_next_proto) to see if this needs to be using a constant-time comparison?
Hi Joth, I think you are the original author of x509_certificate_openssl.cc. Could you take a look at Ryan's comments in that file? Thanks, Jing http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... File net/base/x509_certificate_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:542: return OK; On 2011/11/06 02:39:48, Ryan Sleevi wrote: > random drive by: > > What does it mean if android::VerifyX509CertChain returns VERIFY_OK, but the > VerifyNameMatch (on line 525/526) fails? Right now, that error would be > suppressed. > > As/if/when Chrom[e/ium] implements more stringent checks, this could potentially > be an issue. > > One lesser note (perhaps a TODO, or perhaps never for the android impl.) is to > have the android::VerifyX509CertChain return the constructed certification path. > The reason for this is purely presentation (being able to display the > certificate chain). > > It'd be a low priority, as the display code is not yet wired up, but some of the > unit tests are testing that unrelated, untrusted intermediates are pruned from > the certification path. X509Certificate::Verify() has a default implementation > setting the verified_chain = this, but the expectation is that VerifyInternal() > will update the chain as/when appropriate. I think Joth is the original author of this function. Joth, what do you think about Ryan's comments (including the other two comments below)? http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:802: *out = (unsigned char *) in + 1; On 2011/11/06 02:39:48, Ryan Sleevi wrote: > BUG? Without checking the implementation of SSL_select_next_proto, what happens > if |in| or |inlen| are small (hostile? incorrect?) values. > > Paranoid example, |inlen| == 1, but |*in| == 5. That means line 820 will attempt > to read an invalid range. > > Is this input already validated for correctness? Should/can you document this? > > > style nit: Chromium/Google open-source style uses C++ casts, as convenient as > the C-style cast may be > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Casting > > *out = reinterpret_cast<unsigned char*>(in) + i + 1; > > This is consistent with say, lines 795 and 820. Done. http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:807: j != ssl_config_.next_protos.end(); j++) { On 2011/11/06 02:39:48, Ryan Sleevi wrote: > nit: ++j Done. http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:809: memcmp(&in[i + 1], j->data(), in[i]) == 0) { On 2011/11/06 02:39:48, Ryan Sleevi wrote: > I'm a little nervous, quite possibly unreasonably so, when I see a straight up > memcmp in crypto/security code. While my gut says this probably has no security > relevance (and is thus OK), can you double check with the implementation (say, > SSL_select_next_proto) to see if this needs to be using a constant-time > comparison? Yes, I double checked the implementation from third_party/openssl/openssl/ssl/ssl_lib.c, Line 2798~2832. I use the exact same memcmp as it uses.
Thanks for the drive-by Ryan. http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... File net/base/x509_certificate_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:542: return OK; On 2011/11/06 02:39:48, Ryan Sleevi wrote: > random drive by: > > What does it mean if android::VerifyX509CertChain returns VERIFY_OK, but the > VerifyNameMatch (on line 525/526) fails? Right now, that error would be > suppressed. > oh yes, good spot. To be consistent with the !ANDROID version below, we should just break here, and then at the end of the function:- if (IsCertStatusError(verify_result->cert_status)) return MapCertStatusToNetError(verify_result->cert_status); return OK; } > As/if/when Chrom[e/ium] implements more stringent checks, this could potentially > be an issue. > > One lesser note (perhaps a TODO, or perhaps never for the android impl.) is to > have the android::VerifyX509CertChain return the constructed certification path. > The reason for this is purely presentation (being able to display the > certificate chain). > I've got a task to do that as a follow up (http://b/4290005 for the record, but afraid that tracker is not public) > It'd be a low priority, as the display code is not yet wired up, but some of the > unit tests are testing that unrelated, untrusted intermediates are pruned from > the certification path. X509Certificate::Verify() has a default implementation > setting the verified_chain = this, but the expectation is that VerifyInternal() > will update the chain as/when appropriate. And I think we'll need it for GetCertChainInfo and AppendPublicKeyHashes too. And set is_issued_by_known_root.... I think this thread shows we need some continuous builders and tests running first though http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:667: OSCertHandles cert_handles(intermediate_ca_certs_); On 2011/11/06 02:39:48, Ryan Sleevi wrote: > Two things: > > 1) Why not implement this in terms of GetDEREncoded (line 612), to avoid > diverging DER-accessing implementations? Since you end up copying > |der_cache.data| to a string anyways, there is no extra copy overhead here. I think it's just GetDEREncoded didn't exist when the first version of this was written. I'll do a follow up to refactor this, to avoid iterating it more in this CL http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:671: cert_handles.insert(cert_handles.begin(), cert_handle_); On 2011/11/06 02:39:48, Ryan Sleevi wrote: > 2) This isn't correct how X509Certificate stores its chain, so I'm surprised > this works. > > |intermediate_ca_certs_| contains only the intermediates supplied by the server > - it never contains the end-entity cert (|cert_handle_|). For servers who supply > a certificate chain > 1 (ie: most?), this would fail to consider the EE > certificate. Good spot, thank you. This was broken when it was landed in http://codereview.chromium.org/7538029 (which I never saw to review) (which is also where it was moved to the _android.cc file where it couldn't compile...) In my version it said: if (cert_handles.empty() || cert_handles[0] != cert_handle_) cert_handles.insert(cert_handles.begin(), cert_handle_); As you say intermediate_ca_certs_ never contains cert_handle_ so this guard is redundant, but it does do the right thing at least.
All done. Please take another look. http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... File net/base/x509_certificate_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:542: return OK; On 2011/11/07 10:42:20, joth wrote: > On 2011/11/06 02:39:48, Ryan Sleevi wrote: > > random drive by: > > > > What does it mean if android::VerifyX509CertChain returns VERIFY_OK, but the > > VerifyNameMatch (on line 525/526) fails? Right now, that error would be > > suppressed. > > > oh yes, good spot. > To be consistent with the !ANDROID version below, we should just break here, and > then at the end of the function:- > if (IsCertStatusError(verify_result->cert_status)) > return MapCertStatusToNetError(verify_result->cert_status); > > return OK; > } > Fixed as you said. > > > As/if/when Chrom[e/ium] implements more stringent checks, this could > potentially > > be an issue. > > > > One lesser note (perhaps a TODO, or perhaps never for the android impl.) is to > > have the android::VerifyX509CertChain return the constructed certification > path. > > The reason for this is purely presentation (being able to display the > > certificate chain). > > > > I've got a task to do that as a follow up (http://b/4290005 for the record, but > afraid that tracker is not public) > > > It'd be a low priority, as the display code is not yet wired up, but some of > the > > unit tests are testing that unrelated, untrusted intermediates are pruned from > > the certification path. X509Certificate::Verify() has a default implementation > > setting the verified_chain = this, but the expectation is that > VerifyInternal() > > will update the chain as/when appropriate. > > And I think we'll need it for GetCertChainInfo and AppendPublicKeyHashes too. > And set is_issued_by_known_root.... > I think this thread shows we need some continuous builders and tests running > first though Yes, thank you for the record. http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:667: OSCertHandles cert_handles(intermediate_ca_certs_); On 2011/11/07 10:42:20, joth wrote: > On 2011/11/06 02:39:48, Ryan Sleevi wrote: > > Two things: > > > > 1) Why not implement this in terms of GetDEREncoded (line 612), to avoid > > diverging DER-accessing implementations? Since you end up copying > > |der_cache.data| to a string anyways, there is no extra copy overhead here. > > I think it's just GetDEREncoded didn't exist when the first version of this was > written. I'll do a follow up to refactor this, to avoid iterating it more in > this CL This change seems small so I made it.. Please take a look if it's what you want. http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:671: cert_handles.insert(cert_handles.begin(), cert_handle_); On 2011/11/07 10:42:20, joth wrote: > On 2011/11/06 02:39:48, Ryan Sleevi wrote: > > 2) This isn't correct how X509Certificate stores its chain, so I'm surprised > > this works. > > > > |intermediate_ca_certs_| contains only the intermediates supplied by the > server > > - it never contains the end-entity cert (|cert_handle_|). For servers who > supply > > a certificate chain > 1 (ie: most?), this would fail to consider the EE > > certificate. > > Good spot, thank you. This was broken when it was landed in > http://codereview.chromium.org/7538029 (which I never saw to review) (which is > also where it was moved to the _android.cc file where it couldn't compile...) > > In my version it said: > if (cert_handles.empty() || cert_handles[0] != cert_handle_) > cert_handles.insert(cert_handles.begin(), cert_handle_); > > As you say intermediate_ca_certs_ never contains cert_handle_ so this guard is > redundant, but it does do the right thing at least. Fixed as you said.
Fixes to my comments in x509_certificate_openssl all LG. Some further improvements occur to me, mentioning here for future reference although if rsleevi approves I would land as it is and we can iterate in follow up CLs. http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... File net/base/x509_certificate_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:667: OSCertHandles cert_handles(intermediate_ca_certs_); On 2011/11/08 02:02:17, jingzhao wrote: > This change seems small so I made it.. Please take a look if it's what you want. Thanks, this looks OK. For my own record, when we have test coverage running for this, I would like to go further: void X509Certificate::GetChainDEREncodedBytes(..) chain_bytes->resize(intermediate_ca_certs_.size() + 1); if (!GetDEREncoded(cert_handle_, &chain_bytes->at(0))) return <ERROR> for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) { GetDEREncoded(intermediate_ca_certs_[i], &chain_bytes->at(i + 1)); } } Also the whole function could be inlined at the call-site, avoiding the need for this extra #ifdef in the .h and .cc files. http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:809: memcmp(&in[i + 1], j->data(), in[i]) == 0) { On 2011/11/07 03:42:49, Jing Zhao wrote: > Yes, I double checked the implementation from > third_party/openssl/openssl/ssl/ssl_lib.c, Line 2798~2832. I use the exact same > memcmp as it uses. FWIW, as we have a more expressive class library than in the guts of openssl, we could recode this as: base::StringPiece in_list(in, inlen); for(i = 0; i < in_list.size(); i += in_list[i] + 1) { for(j ... ) { base::StringPiece in_proto(in_list.substring(i + 1, in_list[i])); if (in_proto == *j) { // We found a match... *out = in_proto.data(); *outlen = in_proto.size(); status = OPENSSL_NPN_NEGOTIATED; goto done; } } done: ...
Changes LGTM. A few notes below, but no need for re-review. http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... File net/base/x509_certificate_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:667: OSCertHandles cert_handles(intermediate_ca_certs_); On 2011/11/08 10:05:23, joth wrote: > On 2011/11/08 02:02:17, jingzhao wrote: > > This change seems small so I made it.. Please take a look if it's what you > want. > > Thanks, this looks OK. > > For my own record, when we have test coverage running for this, I would like to > go further: > > void X509Certificate::GetChainDEREncodedBytes(..) > chain_bytes->resize(intermediate_ca_certs_.size() + 1); > if (!GetDEREncoded(cert_handle_, &chain_bytes->at(0))) > return <ERROR> > for (size_t i = 0; i < intermediate_ca_certs_.size(); ++i) { > GetDEREncoded(intermediate_ca_certs_[i], &chain_bytes->at(i + 1)); > } > } > > Also the whole function could be inlined at the call-site, avoiding the need for > this extra #ifdef in the .h and .cc files. Even if not unlined to the callsite, you can move this to the anonymous namespace since GetDEREncoded is public. If you do further tweak this, just use &cert_handles[i], not cert_handles->at(i). I get dinged for using it in style reviews too ;-) http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/21001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:809: memcmp(&in[i + 1], j->data(), in[i]) == 0) { On 2011/11/08 10:05:23, joth wrote: > On 2011/11/07 03:42:49, Jing Zhao wrote: > > > Yes, I double checked the implementation from > > third_party/openssl/openssl/ssl/ssl_lib.c, Line 2798~2832. I use the exact > same > > memcmp as it uses. > > FWIW, as we have a more expressive class library than in the guts of openssl, we > could recode this as: > > base::StringPiece in_list(in, inlen); > for(i = 0; i < in_list.size(); i += in_list[i] + 1) { > for(j ... ) { > base::StringPiece in_proto(in_list.substring(i + 1, in_list[i])); > if (in_proto == *j) { > // We found a match... > *out = in_proto.data(); > *outlen = in_proto.size(); > status = OPENSSL_NPN_NEGOTIATED; > goto done; > } > } > done: > ... +1 to using StringPiece - it would definitely help here for both safety and readability. Not critical for landing this, however, but an ideal cleanup candidate. http://codereview.chromium.org/8429034/diff/33006/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/8429034/diff/33006/build/common.gypi#newcode2070 build/common.gypi:2070: '-lpthread', '-lnss3', '-lnssutil3', '-lsmime3', '-lplds4', '-lplc4', '-lnspr4', Now that you have build/android/ssl, double check to see if these libraries are still being added. nss3 - nspr4 are all NSS libraries - none of which should be included if use_openssl=1 http://codereview.chromium.org/8429034/diff/33006/net/base/x509_certificate_o... File net/base/x509_certificate_openssl.cc (right): http://codereview.chromium.org/8429034/diff/33006/net/base/x509_certificate_o... net/base/x509_certificate_openssl.cc:672: cert_handles.insert(cert_handles.begin(), cert_handle_); This conditional is unnecessary because it will always execute/evaluate as true. You can just delete it and move this statement out to line 671.
Thanks for the review. I'll commit when git try succeeds. http://codereview.chromium.org/8429034/diff/33006/build/common.gypi File build/common.gypi (right): http://codereview.chromium.org/8429034/diff/33006/build/common.gypi#newcode2070 build/common.gypi:2070: '-lpthread', '-lnss3', '-lnssutil3', '-lsmime3', '-lplds4', '-lplc4', '-lnspr4', On 2011/11/08 15:03:23, Ryan Sleevi wrote: > Now that you have build/android/ssl, double check to see if these libraries are > still being added. nss3 - nspr4 are all NSS libraries - none of which should be > included if use_openssl=1 I double checked and it looks like all these are no longer added. Thank you.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jingzhao@chromium.org/8429034/41001
Presubmit check for 8429034-41001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: net/base/dnsrr_resolver_unittest.cc,net/dns/dns_config_service_posix.cc,net/base/host_resolver_impl.cc,net/base/dns_reloader.cc,net/net.gyp,net/base/dnsrr_resolver.cc,net/base/dnsrr_resolver.h,net/base/net_util_unittest.cc,base/base.gypi,net/base/platform_mime_util_linux.cc,net/socket/ssl_client_socket_openssl.cc,net/spdy/spdy_protocol_test.cc,net/base/x509_certificate_openssl.cc Presubmit checks took 2.3s to calculate.
On 2011/11/15 05:52:35, I haz the power (commit-bot) wrote: > Presubmit check for 8429034-41001 failed and returned exit status 1. > > Running presubmit commit checks ... > > ** Presubmit Messages ** > If this change has an associated bug, add BUG=[bug number]. > > If this change requires manual test instructions to QA team, add > TEST=[instructions]. > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for: > net/base/dnsrr_resolver_unittest.cc,net/dns/dns_config_service_posix.cc,net/base/host_resolver_impl.cc,net/base/dns_reloader.cc,net/net.gyp,net/base/dnsrr_resolver.cc,net/base/dnsrr_resolver.h,net/base/net_util_unittest.cc,base/base.gypi,net/base/platform_mime_util_linux.cc,net/socket/ssl_client_socket_openssl.cc,net/spdy/spdy_protocol_test.cc,net/base/x509_certificate_openssl.cc > > Presubmit checks took 2.3s to calculate. Hi Joth, I think I need a LGTM from you to commit this change. Could you help? Thanks, Jing
+wtc for net OWNERS approval.
Hi William, Could you help to review this change? Thanks, Jing
net changes LGTM. http://codereview.chromium.org/8429034/diff/41001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/8429034/diff/41001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:813: j = ssl_config_.next_protos.begin(); nit: Could you indent just this line 4 more spaces? Think that makes it a little more easy to visually parse.
+ Mark for 'base' and 'build' parts
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jingzhao@chromium.org/8429034/54001
Change committed as 110902
Patch Set 12 LGTM. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
