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)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
Attachments
(1 file)
52.88 KB,
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8500034 -
Flags: review?(mmc)
Comment 2•10 years ago
|
||
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+
Comment 3•10 years ago
|
||
We should just have a rule not to have statically initialized class type globals, like the google style guide recommends.
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/76000f9f12da
Comment 6•10 years ago
|
||
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
Assignee | ||
Comment 7•10 years ago
|
||
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
Comment 8•10 years ago
|
||
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.
Description
•