Closed
Bug 1120393
Opened 10 years ago
Closed 10 years ago
Serialize/deserialize nsITransportSecurity.errorCode
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: emk, Assigned: emk)
References
Details
Attachments
(3 files, 1 obsolete file)
4.27 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
2.96 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
3.65 KB,
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
https://hg.mozilla.org/mozilla-central/rev/74b8493b5610 added the errorCode field to the nsITransportSecurity interface, but it didn't change the serialization code.
The SSL error reporting will not obtain correct error codes without this change.
I also fixed a bug of serializing errorMessage. This bug was not uncovered because errorCode was always zero in the e10 child before this change.
Attachment #8547526 -
Flags: review?(dkeeler)
Comment 1•10 years ago
|
||
Comment on attachment 8547526 [details] [diff] [review]
patch
Review of attachment 8547526 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, but needs some changes. r- for now.
::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +322,5 @@
> rv = stream->Write32(mSubRequestsNoSecurity);
> if (NS_FAILED(rv)) {
> return rv;
> }
> + rv = stream->Write32(mErrorCode);
Let's be explicit about this cast: static_cast<uint32_t>(mErrorCode)
(bug 711408 should fix this in a better way, but we should do this for now).
@@ +327,5 @@
> if (NS_FAILED(rv)) {
> return rv;
> }
> + // Do not call formatErrorMessage if mErrorMessageCached is already set
> + // because formatErrorMessage will fail in an e10s child process,
nit: trailing whitespace (also, end the comment with a '.' instead of ',')
Also, does this just fail in e10s, or is it an assertion failure? If the latter, we should check if we're in the child process and return NS_ERROR_FAILURE if so.
@@ +408,5 @@
> + if (NS_FAILED(rv)) {
> + return rv;
> + }
> + // PRErrorCode will be a negative value
> + mErrorCode = errorCode;
We should be explicit about this cast: static_cast<PRErrorCode>(errorCode)
Attachment #8547526 -
Flags: review?(dkeeler) → review-
Assignee | ||
Comment 2•10 years ago
|
||
Resolved review comments.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #1)
> Also, does this just fail in e10s, or is it an assertion failure? If the
> latter, we should check if we're in the child process and return
> NS_ERROR_FAILURE if so.
Good point. I added an error check in formatMessage.
Attachment #8547526 -
Attachment is obsolete: true
Attachment #8548173 -
Flags: review?(dkeeler)
Comment 3•10 years ago
|
||
Comment on attachment 8548173 [details] [diff] [review]
patch v2
Review of attachment 8548173 [details] [diff] [review]:
-----------------------------------------------------------------
Great - r=me with comments addressed. Although, it would be good to test this - can you add a test somewhere that uses add_connection_test and then serializes and deserializes the nsITransportSecurityInfo in the aWithSecurityInfo callback? (i.e. it should check that errorCode survives the process)
::: security/manager/ssl/src/TransportSecurityInfo.cpp
@@ +239,5 @@
> return NS_OK;
> }
>
> + if (XRE_GetProcessType() != GeckoProcessType_Default) {
> + result.Truncate();
We should just hoist this Truncate to the top of the function and remove it from the if (errorCode == 0) block.
@@ +331,5 @@
> if (NS_FAILED(rv)) {
> return rv;
> }
> + // Do not call formatErrorMessage if mErrorMessageCached is already set
> + // because formatErrorMessage will fail in an e10s child process.
Actually, I don't think this comment is necessary.
Attachment #8548173 -
Flags: review?(dkeeler) → review+
Comment 4•10 years ago
|
||
Actually, it would be more useful to check that the error code is serialized/deserialized correctly in the SSL error reporting test. Is that test enabled for e10s?
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> Actually, it would be more useful to check that the error code is
> serialized/deserialized correctly in the SSL error reporting test. Is that
> test enabled for e10s?
Currently the error code will not survive even in non-e10s mode because the SSL error reporting will explicitly serialize/deserialize TranportSecurityInfo on sending the event regardless whether the e10s is enabled [1][2].
Looks like browser_bug846489.js did not test for the error code at all [3]. I'll add a test for it.
[1] https://hg.mozilla.org/mozilla-central/annotate/63006936ab99/browser/base/content/content.js#l268
[2] https://hg.mozilla.org/mozilla-central/annotate/63006936ab99/browser/base/content/browser.js#l2641
[3] https://hg.mozilla.org/mozilla-central/rev/287ddaf9b9df
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8548987 -
Flags: review?(dkeeler)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8548990 -
Flags: review?(felipc)
Assignee | ||
Updated•10 years ago
|
Attachment #8548990 -
Attachment is patch: true
Comment 8•10 years ago
|
||
Comment on attachment 8548987 [details] [diff] [review]
Unit test
Review of attachment 8548987 [details] [diff] [review]:
-----------------------------------------------------------------
Great - r=me.
::: security/manager/ssl/tests/unit/test_cert_chains.js
@@ +50,5 @@
> + let deserialized = serHelper.deserializeObject(serialized);
> + deserialized.QueryInterface(Ci.nsITransportSecurityInfo);
> + do_check_eq(securityInfo.securityState, deserialized.securityState);
> + do_check_eq(securityInfo.errorMessage, deserialized.errorMessage);
> + do_check_eq(securityInfo.errorCode, deserialized.errorCode);
Let's also do_check_neq(deserialized.errorCode, 0); for thoroughness.
Attachment #8548987 -
Flags: review?(dkeeler) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8548990 [details] [diff] [review]
mochitest_browser
Review of attachment 8548990 [details] [diff] [review]:
-----------------------------------------------------------------
I'm just curious why is this necessary
Attachment #8548990 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 10•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=553a27466906
(In reply to David Keeler [:keeler] (use needinfo?) from comment #8)
> ::: security/manager/ssl/tests/unit/test_cert_chains.js
> @@ +50,5 @@
> > + let deserialized = serHelper.deserializeObject(serialized);
> > + deserialized.QueryInterface(Ci.nsITransportSecurityInfo);
> > + do_check_eq(securityInfo.securityState, deserialized.securityState);
> > + do_check_eq(securityInfo.errorMessage, deserialized.errorMessage);
> > + do_check_eq(securityInfo.errorCode, deserialized.errorCode);
>
> Let's also do_check_neq(deserialized.errorCode, 0); for thoroughness.
I did not add this because:
1. It already tested do_chack_eq(securityInfo.errorCode, <nonzero>) and do_chack_eq(securityInfo.errorCode, deserialized.errorCode)
2. deserialized.errorCode can be zero if the connection succeeds.
Instead, I added expectedErrorCode parameter to the function, moved the do_chack_eq into test_security_info_serialization, and added a test for the successful connection case.
In general, we should avoid do_check_neq if we know what is the expected value.
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to :Felipe Gomes from comment #9)
> I'm just curious why is this necessary
Because it would have caught this earlier and SSL error report will be less useful if the server will always see zero for all errors.
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6da326356fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/48b98d0a4fa4
https://hg.mozilla.org/integration/mozilla-inbound/rev/50b459cff425
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/b6da326356fb
https://hg.mozilla.org/mozilla-central/rev/48b98d0a4fa4
https://hg.mozilla.org/mozilla-central/rev/50b459cff425
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•