Closed Bug 1214981 Opened 4 years ago Closed 4 years ago

Intermittent test_weak_crypto.js | - Actual and expected connection result should match - 2153394174 == 2152398868

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox44 --- fixed

People

(Reporter: philor, Assigned: emk)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

patch for checkin
4.20 KB, patch
emk
: review+
Details | Diff | Splinter Review
Duplicate of this bug: 1215268
This has been failing quite frequently (at least two hundred times in two days) ever since bug 1168635 landed. I'd like to avoid having to back that out since you've landed UI on top of it. Could you look into this, Masatoshi-san?
Flags: needinfo?(VYV03354)
I'm looking into this.
Flags: needinfo?(VYV03354)
But please disable the test instead of reverting the entire patch. Looks like this is an existing bug. It's just no test was present before bug 1215268.
Attached patch Disable output stream buffering (obsolete) — — Splinter Review 
checkHandshake() will return a fake NS_ERROR_NET_RESET only once when insecure fallback is needed.
https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/nsNSSIOLayer.cpp?rev=bd6226d81b60#1248
nsSocketInputStream and nsSocketOutputStream will store the error code separately.
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsSocketTransport2.h?rev=41dea9df27ed&mark=67-67#64
https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsSocketTransport2.h?rev=41dea9df27ed&mark=101-101#98
So if nsSocketInputStream::Read() and nsSocketOutputStream::Write() is called alternately, the correct error code may not be stored.
Why Read() can be called before Write()? I couldn't investigate it completely, but apparently a buffered stream is doing something weird. If the buffering is enabled, Write() will never fail as long as the written size is smaller than the buffer. So the test couldn't receive error until onInputStreamReady() is fired.
I disabled the buffering to workaround the problem.

Android s4 succeeded 10/10 with this patch. Hopefully this patch can avoid intermittent failures...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=617ac3513df5
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8675256 - Flags: review?(dkeeler)
Blocks: 1168635
Comment on attachment 8675256 [details] [diff] [review]
Disable output stream buffering

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

For the moment, this looks like an ok solution. However, I do think there is a bug here that we should investigate in the future. r=me with comments addressed.

::: security/manager/ssl/tests/unit/test_weak_crypto.js
@@ +149,5 @@
> +          output.close();
> +          deferred.resolve();
> +          return;
> +        }
> +        equal(Cr.NS_OK, expectedResult, "Connection should success");

nit: s/success/succeed/

@@ -151,5 @@
>          input = transport.openInputStream(0, 0, 0);
>          input.asyncWait(handler, 0, 0, Services.tm.currentThread);
>        } catch (e) {
> -        let errorCode = -1 * (e.result & 0xFFFF);
> -        if (errorCode == SSL_ERROR_BAD_CERT_ALERT) {

If we're getting rid of SSL_ERROR_BAD_CERT_ALERT, we don't need to define it earlier on in startClient (same with SSL_ERROR_BASE as a result).
Attachment #8675256 - Flags: review?(dkeeler) → review+
Attachment #8675256 - Attachment is obsolete: true
Attachment #8677137 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f81fb04f1189
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Filed bug 1217368 to track the root cause.
You need to log in before you can comment on or make changes to this bug.