Remove mozilla::pkix's dependencies on PLArenaPool

RESOLVED FIXED in mozilla35

Status

()

Core
Security: PSM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 attachments)

55.98 KB, patch
keeler
: review+
Details | Diff | Splinter Review
15.59 KB, patch
keeler
: review+
Details | Diff | Splinter Review
3.14 KB, patch
keeler
: review+
Details | Diff | Splinter Review
12.71 KB, patch
keeler
: review+
Details | Diff | Splinter Review
4.35 KB, patch
keeler
: review+
Details | Diff | Splinter Review
2.56 KB, patch
keeler
: review+
Details | Diff | Splinter Review
13.14 KB, patch
keeler
: review+
Details | Diff | Splinter Review
4.92 KB, patch
keeler
: review+
Details | Diff | Splinter Review
4.62 KB, patch
keeler
: review+
Details | Diff | Splinter Review
40.13 KB, patch
keeler
: review+
Details | Diff | Splinter Review
10.18 KB, patch
keeler
: review+
Details | Diff | Splinter Review
4.91 KB, patch
keeler
: review+
Details | Diff | Splinter Review
2.48 KB, patch
keeler
: review+
Details | Diff | Splinter Review
46.96 KB, patch
keeler
: review+
Details | Diff | Splinter Review
32.16 KB, patch
keeler
: review+
Details | Diff | Splinter Review
5.97 KB, patch
keeler
: review+
Details | Diff | Splinter Review
2.03 KB, patch
keeler
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Depends on: 1059928
Created attachment 8482559 [details] [diff] [review]
Part 1: Stop using NSS to encode names in tests
Attachment #8482559 - Flags: review?(dkeeler)
Created attachment 8482561 [details] [diff] [review]
Part 2: Stop using NSS to encode integers and serial number
Attachment #8482561 - Flags: review?(dkeeler)
Created attachment 8482562 [details] [diff] [review]
Part 3: Stop using PLArenaPool for BitString encoding
Attachment #8482562 - Flags: review?(dkeeler)
Created attachment 8482563 [details] [diff] [review]
Part 4: Stop using PLArenaPool for time encoding
Attachment #8482563 - Flags: review?(dkeeler)
Created attachment 8482567 [details] [diff] [review]
Part 5: Remove InitInputFromSECItem
Attachment #8482567 - Flags: review?(dkeeler)
Created attachment 8482568 [details] [diff] [review]
Part 6: Stop using PLArenaPool for boolean encoding
Attachment #8482568 - Flags: review?(dkeeler)
Created attachment 8482569 [details] [diff] [review]
Part 7: Stop using PLArenaPool for SignedData encoding
Attachment #8482569 - Flags: review?(dkeeler)
Created attachment 8482570 [details] [diff] [review]
Part 8: Stop using PLArenaPool for CertID encoding
Attachment #8482570 - Flags: review?(dkeeler)
Created attachment 8482571 [details] [diff] [review]
Part 9: Stop using PLArenaPool for SingleResponse encoding
Attachment #8482571 - Flags: review?(dkeeler)
Created attachment 8482574 [details] [diff] [review]
Part 10: Stop using PLArenaPool for extension encoding
Attachment #8482574 - Flags: review?(dkeeler)
Created attachment 8482575 [details] [diff] [review]
Part 11: Stop using PLArenaPool for TBSCertificate and SignedData encoding
Attachment #8482575 - Flags: review?(dkeeler)
Created attachment 8482576 [details] [diff] [review]
Part 12: Stop using PLArenaPool for ResponseData encoding
Attachment #8482576 - Flags: review?(dkeeler)
Created attachment 8482578 [details] [diff] [review]
Part 13: Remove Output class
Attachment #8482578 - Flags: review?(dkeeler)
Created attachment 8482579 [details] [diff] [review]
Part 14: Stop using PLArenaPool in CreateEncodedCertificate
Attachment #8482579 - Flags: review?(dkeeler)
Created attachment 8482581 [details] [diff] [review]
Part 15: Stop using PLArenaPool in CreateEncodedOCSPResponse
Attachment #8482581 - Flags: review?(dkeeler)
Created attachment 8482582 [details] [diff] [review]
Part 16: Stop using PLArenaPool in pkixocsp_CreateEncodedOCSPRequest
Attachment #8482582 - Flags: review?(dkeeler)
Created attachment 8482583 [details] [diff] [review]
Part 17: Use now-unused PLArenaPool infrastructure
Attachment #8482583 - Flags: review?(dkeeler)
Comment on attachment 8482559 [details] [diff] [review]
Part 1: Stop using NSS to encode names in tests

Review of attachment 8482559 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with comments addressed.

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +167,2 @@
>  
> +  // endEntityCertID references items owned by arena, rootSPKI, and rootNameDER.

nit: this comment should probably stay directly above endEntityCertID

@@ +614,5 @@
>  
>  TEST_F(pkixocsp_VerifyEncodedResponse_DelegatedResponder,
>         good_indirect_wrong_eku)
>  {
> +

nit: not sure this blank line is necessary

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +139,5 @@
> +    result.push_back(value.length());
> +  } else if (value.length() < 65536) {
> +    result.push_back(0x82u);
> +    result.push_back(static_cast<uint8_t>(value.length() / 256));
> +    result.push_back(static_cast<uint8_t>(value.length()));

Hmmm - intuitively it seems like a good idea to explicitly include "value.length() % 256" rather than having the cast do it implicitly.

@@ +171,5 @@
>    {
>      assert(item);
>      assert(item->data);
> +    contents.append(item->data, item->len);
> +    return Success; // XXX: return type should be void

Should this be changed now? Or do later patches take care of this/obviate the need?

@@ -210,5 @@
> -  size_t numItems;
> -  size_t length;
> -
> -  Output(const Output&) /* = delete */;
> -  void operator=(const Output&) /* = delete */;

Do we not still want to delete these?

@@ +720,5 @@
>                           SignatureAlgorithm signatureAlgorithm,
>                           /*out*/ ScopedSECKEYPrivateKey& privateKeyResult)
>  {
>    assert(arena);
> +  if (!arena) {

Should we also check/assert that issuerNameDER, subjectNameDER != ENCODING_FAILED?

@@ +781,2 @@
>    assert(subjectPublicKey);
> +  if (!arena || !subjectPublicKey) {

Same idea here
Attachment #8482559 - Flags: review?(dkeeler) → review+
Comment on attachment 8482561 [details] [diff] [review]
Part 2: Stop using NSS to encode integers and serial number

Review of attachment 8482561 [details] [diff] [review]:
-----------------------------------------------------------------

Great!
Attachment #8482561 - Flags: review?(dkeeler) → review+
Attachment #8482562 - Flags: review?(dkeeler) → review+
Attachment #8482563 - Flags: review?(dkeeler) → review+
Attachment #8482567 - Flags: review?(dkeeler) → review+
Comment on attachment 8482568 [details] [diff] [review]
Part 6: Stop using PLArenaPool for boolean encoding

Review of attachment 8482568 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +294,5 @@
>    return TLV(der::BIT_STRING, prefixed);
>  }
>  
> +static ByteString
> +Boolean(bool value)

Interesting - I guess we never call Boolean(false)? (I imagine this is due to default values. Anyway, it's probably not worth changing it to always encode true.)
Attachment #8482568 - Flags: review?(dkeeler) → review+
Attachment #8482569 - Flags: review?(dkeeler) → review+
Attachment #8482570 - Flags: review?(dkeeler) → review+
Comment on attachment 8482559 [details] [diff] [review]
Part 1: Stop using NSS to encode names in tests

Review of attachment 8482559 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +171,5 @@
>    {
>      assert(item);
>      assert(item->data);
> +    contents.append(item->data, item->len);
> +    return Success; // XXX: return type should be void

One of the patches in this bug completely removes the Output class, so it doesn't matter.

@@ -210,5 @@
> -  size_t numItems;
> -  size_t length;
> -
> -  Output(const Output&) /* = delete */;
> -  void operator=(const Output&) /* = delete */;

It is no longer necessary to delete them because the defaults do the correct thing now. Also, it doesn't matter because Output gets removed a few patches later.

@@ +720,5 @@
>                           SignatureAlgorithm signatureAlgorithm,
>                           /*out*/ ScopedSECKEYPrivateKey& privateKeyResult)
>  {
>    assert(arena);
> +  if (!arena) {

The assertions were to avoid null pointer dereferencing. I'd like to avoid adding new assertions that check against ENCODING_FAILED because it may be useful for future tests to pass in empty values as a way of testing what happens when a mandatory field like serialNumber is missing.

@@ +781,2 @@
>    assert(subjectPublicKey);
> +  if (!arena || !subjectPublicKey) {

Dittyo.
Attachment #8482571 - Flags: review?(dkeeler) → review+
Comment on attachment 8482574 [details] [diff] [review]
Part 10: Stop using PLArenaPool for extension encoding

Review of attachment 8482574 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +156,2 @@
>      extension.critical = (aORT == ORTCriticalExtension);
> +    extension.value.push_back(0x05);

This was supposed to be NULL, if I recall correctly - maybe use NULLTag from pkixder.h?
Attachment #8482574 - Flags: review?(dkeeler) → review+
Attachment #8482575 - Flags: review?(dkeeler) → review+
Attachment #8482576 - Flags: review?(dkeeler) → review+
Attachment #8482578 - Flags: review?(dkeeler) → review+
Comment on attachment 8482579 [details] [diff] [review]
Part 14: Stop using PLArenaPool in CreateEncodedCertificate

Review of attachment 8482579 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/lib/pkixtestutil.h
@@ +197,5 @@
>                                 // regardless of if there are any actual
>                                 // extensions.
>    ScopedSECKEYPrivateKey signerPrivateKey;
>    bool badSignature; // If true, alter the signature to fail verification
> +  const ByteString* certs; // optional; terminated by an empty string

maybe add that this is an array
Attachment #8482579 - Flags: review?(dkeeler) → review+
Comment on attachment 8482581 [details] [diff] [review]
Part 15: Stop using PLArenaPool in CreateEncodedOCSPResponse

Review of attachment 8482581 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +180,5 @@
>  
> +  SECItem item = {
> +    siBuffer,
> +    const_cast<uint8_t*>(response.data()),
> +    static_cast<unsigned int>(response.length())

There may be some compilers (e.g. what we use on some b2g platforms) that don't support this syntax (I may be misremembering, however).

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ -627,5 @@
>  // Test that signature of OCSP response signer cert is verified
>  TEST_F(pkixocsp_VerifyEncodedResponse_DelegatedResponder, good_tampered_eku)
>  {
> -  Input response(CreateEncodedIndirectOCSPSuccessfulResponse(
> -                         "CN=good_indirect_tampered_eku",

hmmm - should this have been "good_indirect_tampered_eku" in an earlier patch?
Attachment #8482581 - Flags: review?(dkeeler) → review+
Attachment #8482582 - Flags: review?(dkeeler) → review+
Attachment #8482583 - Flags: review?(dkeeler) → review+
Comment on attachment 8482574 [details] [diff] [review]
Part 10: Stop using PLArenaPool for extension encoding

Review of attachment 8482574 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +156,2 @@
>      extension.critical = (aORT == ORTCriticalExtension);
> +    extension.value.push_back(0x05);

I added comments to clarify that. We cannot include pkixder.h here, because pkixder.h is private to mozilla::pkix.
Comment on attachment 8482581 [details] [diff] [review]
Part 15: Stop using PLArenaPool in CreateEncodedOCSPResponse

Review of attachment 8482581 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp
@@ +180,5 @@
>  
> +  SECItem item = {
> +    siBuffer,
> +    const_cast<uint8_t*>(response.data()),
> +    static_cast<unsigned int>(response.length())

This isn't a "static const" initialization so it doesn't break on any compilers (verified on tryserver).

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ -627,5 @@
>  // Test that signature of OCSP response signer cert is verified
>  TEST_F(pkixocsp_VerifyEncodedResponse_DelegatedResponder, good_tampered_eku)
>  {
> -  Input response(CreateEncodedIndirectOCSPSuccessfulResponse(
> -                         "CN=good_indirect_tampered_eku",

Yes; I fixed part 1.
Duplicate of this bug: 1061067
Depends on: 1095926
You need to log in before you can comment on or make changes to this bug.