If the version field is omitted from the certificate, the default

If the version field is omitted from the certificate, the default
value is v1(0).

R=mattm@chromium.org
BUG=110132
TEST=unit_tests --gtest_filter=X509CertificateModelTest.GetVersionOmitted

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

[wtc, 2012-05-18 01:15:57]

mattm: please review.

Is it easy to add a unit test for this code?

https://chromiumcodereview.appspot.com/10391197/diff/1/chrome/common/net/x509...
File chrome/common/net/x509_certificate_model_nss.cc (left):

https://chromiumcodereview.appspot.com/10391197/diff/1/chrome/common/net/x509...
chrome/common/net/x509_certificate_model_nss.cc:130: version != ULONG_MAX)
I guess the version != ULONG_MAX check here is to work
around a bug in SEC_ASN1DecodeInteger when
    cert_handle->version.data != NULL
    cert_handle->version,len == 0

In that case, SEC_ASN1DecodeInteger returns SECSuccess
and stores either 0 or (unsigned long)-1 in the output
argument.

In the new code, we don't call SEC_ASN1DecodeInteger
if cert_handle->version.len == 0, so this workaround is
not necessary.

[mattm, 2012-05-18 03:19:50]

lgtm

On 2012/05/18 01:15:57, wtc wrote:
> mattm: please review.
> 
> Is it easy to add a unit test for this code?

It should be easy enough to add a test in
chrome/common/net/x509_certificate_model_unittest.cc if we add a v1 cert data
file to the test data.

https://chromiumcodereview.appspot.com/10391197/diff/1/chrome/common/net/x509...
File chrome/common/net/x509_certificate_model_nss.cc (left):

https://chromiumcodereview.appspot.com/10391197/diff/1/chrome/common/net/x509...
chrome/common/net/x509_certificate_model_nss.cc:130: version != ULONG_MAX)
On 2012/05/18 01:15:57, wtc wrote:
> I guess the version != ULONG_MAX check here is to work
> around a bug in SEC_ASN1DecodeInteger when
>     cert_handle->version.data != NULL
>     cert_handle->version,len == 0

Honestly I can't remember why it's that way, but that sounds likely.

> In that case, SEC_ASN1DecodeInteger returns SECSuccess
> and stores either 0 or (unsigned long)-1 in the output
> argument.
> 
> In the new code, we don't call SEC_ASN1DecodeInteger
> if cert_handle->version.len == 0, so this workaround is
> not necessary.

sounds good.  Do we need to check version.data == NULL also or will len always
be 0 in that case?

[wtc, 2012-05-18 21:47:13]

mattm: please review patch set 2.  I added a unit test.  Thanks.

https://chromiumcodereview.appspot.com/10391197/diff/1/chrome/common/net/x509...
File chrome/common/net/x509_certificate_model_nss.cc (left):

https://chromiumcodereview.appspot.com/10391197/diff/1/chrome/common/net/x509...
chrome/common/net/x509_certificate_model_nss.cc:130: version != ULONG_MAX)

On 2012/05/18 03:19:50, mattm wrote:
>
> sounds good.  Do we need to check version.data == NULL also or will len always
> be 0 in that case?

In my testing with several recent versions of NSS, I always
see data == NULL, len == 0.  So it should be sufficient to
just check version.len == 0.

[mattm, 2012-05-18 22:01:07]

lgtm

[commit-bot: I haz the power, 2012-05-19 00:00:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/10391197/4001

[commit-bot: I haz the power, 2012-05-19 01:35:44]

Change committed as 138005