1114703 - Clean up or remove mozilla::pkix's bind.h

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Daniel Holbert [:dholbert] from bug 1114671 comment 4)
> (Tangent: I noticed that this new Bind5 version of bind() has different args
> than the other versions of bind().  The Bind5 version has args "B1 b1, B2
> b2," etc., whereas the Bind4 version of bind() has args "const B1& b1, const
> B2& b2". I wonder if that inconsistency is there for a reason? Presumably we
> should use const-references if possible. That should probably happen in a
> separate bug/patch, though, and it's not currently causing build breakage
> like this other issue, so I'm not jumping on it. :))

This specific hack was a result of me learning that I could write fewer Bind classes if I had the user of Bind decide which things should be const or not. I'd like to go back and redo the other definitions to be like Bind5.

However, this whole file is a horrible hack. We should just switch it to use variadic templates, but I think we need compilers to be upgraded first. And, once they are upgraded, we'll be able to use lambdas and so we won't need bind.h at all any more.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: no-more-bind.patch

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Here's the try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d4fa1e41bbf

Not sure what happened with Linux Debug. I retriggered it to see if it was intermittent.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8552562 [details] [diff] [review]
no-more-bind.patch

Clearing review request. I wrote some more code that uses lambdas, and I realized that I should be using formatting more like the formatting uesd for JS lambdas, instead of the harder-to-read formatting I used here. I'll upload a reformatted patch.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: no-more-bind.patch [v2]

Here's the patch with the updated formatting, consistent with how we do lambdas in JS.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8552799 [details] [diff] [review]
no-more-bind.patch [v2]

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

Much better, thanks for making these changes. There was one change I didn't understand.

::: security/pkix/lib/pkixcheck.cpp
@@ +313,5 @@
> +      Result rv = der::OptionalBoolean(r, isCA);
> +      if (rv != Success) {
> +        return rv;
> +      }
> +      // TODO(bug 985025): If isCA is false, pathLenConstraint

Did you mean to include these changes? They seem unrelated.

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8552799 [details] [diff] [review]
no-more-bind.patch [v2]

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

::: security/pkix/lib/pkixcheck.cpp
@@ +313,5 @@
> +      Result rv = der::OptionalBoolean(r, isCA);
> +      if (rv != Success) {
> +        return rv;
> +      }
> +      // TODO(bug 985025): If isCA is false, pathLenConstraint

This code is the code from the DecodeBasicConstraints function. I could have written this as:

  Result rv = der::Nested(input, der::SEQUENCE,
                          [&isCA, &[pathLenConstraint](Reader& r) {
    return DecodeBasicConstraints(r, isCA, pathLenConstraint);
  });

However, I think putting DecodeBasicConstraints's code inline makes more sense, considering we're using a lambda anyway.

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8552799 [details] [diff] [review]
no-more-bind.patch [v2]

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

Thanks for explaining.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks for the quick review turnaround!

https://hg.mozilla.org/integration/mozilla-inbound/rev/49651d30167a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/49651d30167a