Closed Bug 1077859 Opened 10 years ago Closed 10 years ago

mozilla::pkix::test::ENCODING_FAILED is unsafe to use as defined

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

Initialization order of global/static variables is undefined in C++. In particular, if you have a static variable, and you initialize it with a function call, and that function call depends on the value of another global/static variable, then the function might execute before the value of the variable it depends on has been initialized. In our case, this occurs when we have static variables in tests that are initialized with functions like CNToDERName, because CNToDERName depends on the value of ENCODING_FAILED.

Additionally, the fact that many functions like TLV may fail by returning ENCODING_FAILED ("") is extremely inconvenient, if you need to write tests that call TLV or functions that call TLV. TLV only fails when the input is too large, which never will happen in our tests. Therefore, I changed TLV and a few other commonly-called functions to abort() instead of returning ENCODING_FAILED. This reduces test boilerplate significantly in some cases, such as the tests for bug 1063281.
Comment on attachment 8500034 [details] [diff] [review]
ENCODING_FAILED.patch

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

Looks fine to me.

::: security/pkix/test/lib/pkixtestutil.cpp
@@ +187,5 @@
>  Integer(long value)
>  {
>    if (value < 0 || value > 127) {
>      // TODO: add encoding of larger values
> +    // It is MUCH more convenient for TLV for be infallible than for it to have

s/TLV/Integer
Attachment #8500034 - Flags: review?(mmc) → review+
We should just have a rule not to have statically initialized class type globals, like the google style guide recommends.
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #3)
> We should just have a rule not to have statically initialized class type
> globals, like the google style guide recommends.

We already have that rule, at least for non-test code. We could do that for test code too, but that will make some tests much more awkward to write. For example, the tests for bug 1063281 really benefit in readability from being able to construct std::basic_string instances in static initializers.

Thanks for the review! I corrected the typo you pointed out.
sorry had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=8847ac2e2a53 since one of this 3 checkins made XPCshell tests more failing frequently like the issues in bug 964673 as example
Thanks for backing out the patches Cartsen. I relanded this, because it wasn't causing the failures you cited.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa723c227218
https://hg.mozilla.org/mozilla-central/rev/fa723c227218
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.