Closed Bug 1296716 Opened 3 years ago Closed 3 years ago
Uninitialised value uses somehow relating to ns
Socket Transport Service::Analyze Connection
STR: DISPLAY=:1.0 G_SLICE=always-malloc \ ./mach mochitest --valgrind=vTRUNK \ --valgrind-args=--num-transtab-sectors=24,--px-default=allregs-at-mem-access,--px-file-backed=unwindregs-at-mem-access,--fair-sched=yes,--fullpath-after=/MOZ/,--trace-children=yes,--show-mismatched-frees=no,--track-origins=no \ browser/components/contextualidentity/test/browser/browser_aboutURLs.js \ 2>&1 | tee spew-14-08-mc Reproduces with an -O2 build on Haswell at 2.5 GHz with --track-origins=no but did not show with --track-origins=yes (which is much slower). Had to run at 3.7 GHz to get that. This makes me wonder if it is somehow timing dependent. The uninitialised values seem to seep into JS land too, although no origins are shown for those uses, so I can't be 100% sure it's the same problem.
Patrick suggested on irc that this might be due to the call from nsSocketTransportService::AnalyzeConnection to PR_NetAddrToString silently failing. It appears to be responsible for initialising |peer_addr|, and its return value isn't checked.
or the getpeername stuff in the same function..
Zero-initialise |peer_addr|, so that failures in PR_GetPeerName/PR_NetAddrToString don't leave it in an undefined state.
Assignee: nobody → jseward
Status: NEW → ASSIGNED
Nick, would you be able to comment on this? Honza and Patrick both suggested that merely zero-initialising the relevant struct as per the patch in comment 4 might not be a good approach, and it would be better to add missing error-checking.
So if it were all me, I'd do both - zero-initialize (which is always a good idea, IMHO), *and* add error checking to PR_GetPeerAddr and PR_NetAddrToString calls. It's probably not a huge deal if we return early because those fail (we already have an early return a few lines up). That said, I don't know a whole ton about the networking dashboard code, so there may be some subtleties I'm missing. Let's ask Valentin, who wrote all this :)
Flags: needinfo?(hurley) → needinfo?(valentin.gosu)
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #6) > So if it were all me, I'd do both - zero-initialize (which is always a good > idea, IMHO), *and* add error checking to PR_GetPeerAddr and > PR_NetAddrToString calls. It's probably not a huge deal if we return early > because those fail (we already have an early return a few lines up). I agree. If an error occurs, we have to choose between not reporting that entry in about:networking via an early return, or report a corrupted ( or empty value if we do 0 init ) value. So any or both of the two fixes are fine with me.
Zeroes out the struct and also checks for error returns from the two function calls discussed.
Attachment #8789262 - Flags: review?(valentin.gosu)
Attachment #8789262 - Flags: review?(valentin.gosu) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/b02007bc6c6a Uninitialised value uses somehow relating to nsSocketTransportService::AnalyzeConnection. email@example.com.
Here are two other calls to PR_NetAddrToString() that don't check its return value: http://searchfox.org/mozilla-central/source/security/manager/ssl/TransportSecurityInfo.cpp#676 http://searchfox.org/mozilla-central/source/security/manager/ssl/SSLServerCertVerification.cpp#1008
You need to log in before you can comment on or make changes to this bug.