Closed Bug 1313385 Opened 7 years ago Closed 7 years ago

WebSocketImpl::WebSocketImpl does not initialise mSecure


(Core :: DOM: Core & HTML, defect)

Not set



Tracking Status
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 + fixed
firefox52 - fixed


(Reporter: jseward, Assigned: baku)


(Keywords: csectype-uninitialized, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main51+])


(2 files)

This is dynamically observable in our test suites.  STR to follow.

Setting as sec-sensitive because it seems to me to be ungood to be
confused about whether a connection is secure or not.  Please declassify
as appropriate.
Attached file Valgrind complaint
Looking at dom/base/WebSocket.cpp, it seems that class WebSocketImpl
has a |bool mSecure|, but |explicit WebSocketImpl(WebSocket* aWebSocket)|
does not give an initial value for it.  I am hypothesising that this is
the cause of the reported complaint.
Group: core-security → dom-core-security
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Attached patch ws.patchSplinter Review
Attachment #8805213 - Flags: review?(kyle)
Attachment #8805213 - Flags: review?(kyle) → review+
Assignee: nobody → amarchesini
(In reply to Andrea Marchesini [:baku] from comment #3)
> Created attachment 8805213 [details] [diff] [review]
> ws.patch

Verified as fixing the complaint.

The test is devtools/server/tests/mochitest/test_websocket-server.html

FTR, command line I used is

DISPLAY=:1.0 G_SLICE=always-malloc ./mach mochitest -f chrome --debugger=v312BRANCH \
  --debugger-args="--fair-sched=yes --smc-check=all-non-file \
  --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc-may2015.supp --error-limit=no \
  --trace-children=yes \
  --child-silent-after-fork=yes --trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java \
  --num-transtab-sectors=24 --tool=memcheck --freelist-vol=300000000 --redzone-size=64 \
  --gen-suppressions=no --px-default=allregs-at-mem-access \
  --px-file-backed=unwindregs-at-mem-access --stats=no --show-mismatched-frees=no \
  --fullpath-after=/MOZ/ --num-callers=20 --track-origins=no" --keep-open=no \
  devtools/server/tests/mochitest/test_websocket-server.html 2>&1 | tee spew-01-06-mc
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Tracking 52- since this is now resolved fixed.
Hi :dveditz, 
Since 51 is also affected, is this worth uplifting to 51?
Flags: needinfo?(dveditz)
Track 51+ as sec-moderate.
sure, this is a trivially safe fix to uplift to 51 (and ESR for that matter).
Flags: needinfo?(dveditz)
Hi :baku,
could you please nominate this uplift to Aurora51 & ESR?
Flags: needinfo?(amarchesini)
Comment on attachment 8805213 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: WebSocket ported to webIDL
[User impact if declined]: mSecure is not initialized in the CTOR. In theory until the initialization is completed in other methods, this value is unset.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: none
[String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8805213 - Flags: approval-mozilla-aurora?
Comment on attachment 8805213 [details] [diff] [review]

Fix a sec-moderate. Take it in 51 aurora.
Attachment #8805213 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: dom-core-security → core-security-release
This may as well wait for the ESR that will go out with 51 (45.7.0).
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+]
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.