Closed Bug 1296716 Opened 3 years ago Closed 3 years ago

Uninitialised value uses somehow relating to nsSocketTransportService::AnalyzeConnection

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: jseward, Assigned: jseward)

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 1 obsolete file)

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..
Attached patch bug1296716-1.cset (obsolete) — Splinter Review
Zero-initialise |peer_addr|, so that failures in 
PR_GetPeerName/PR_NetAddrToString don't leave it in an undefined state.
Attachment #8783160 - Flags: review?(mcmanus)
Assignee: nobody → jseward
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
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.
Flags: needinfo?(hurley)
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.
Flags: needinfo?(valentin.gosu)
Zeroes out the struct and also checks for error returns from the two
function calls discussed.
Attachment #8783160 - Attachment is obsolete: true
Attachment #8783160 - Flags: review?(mcmanus)
Attachment #8789262 - Flags: review?(valentin.gosu)
Attachment #8789262 - Flags: review?(valentin.gosu) → review+
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b02007bc6c6a
Uninitialised value uses somehow relating to nsSocketTransportService::AnalyzeConnection.  r=valentin.gosu@gmail.com.
https://hg.mozilla.org/mozilla-central/rev/b02007bc6c6a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.