Closed Bug 1252073 Opened 6 years ago Closed 6 years ago

Uninitialised value uses in mozilla::EncodingConstraints::operator==

Categories

(Core :: WebRTC: Signaling, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jseward, Assigned: jseward)

Details

Attachments

(2 files)

STR: do a build with --enable-valgrind --disable-jemalloc, then:

DISPLAY=:1.0 ./mach mochitest --keep-open=false --valgrind=vMOZHX \
  --valgrind-args=--fullpath-after=MOCHI/,--track-origins=yes,--num-transtab-sectors=24,--px-default=allregs-at-mem-access,--px-file-backed=unwindregs-at-mem-access,--fair-sched=yes \
  -f plain \
  dom/media/testsmochitesttest_peerConnection_verifyVideoAfterRenegotiation.html
Attached file Valgrind complaint
My best guess about this is as follows.  275908:3baf48e3f758 (bug 1192390)
introduced the class EncodingConstraints as it stands now, except without
"double scaleDownBy".  It also introduced a memcmp-based operator==, which
strikes me as rather dubious, but it happened to work, because the vtbl
pointer and all the uint32_t fields are packed back-to-back with no holes.

Some time later, 285389:607884f102a7 (bug 1244913) added "double scaleDownBy".
This requires 64-bit alignment, whereas all the other 9 fields are uint32_t and
require only 32-bit alignment.  The implication is that the compiler is forced
to put a 32-bit padding field after "uint32_t maxDpb" to create the correct
alignment, and memcmp now falls across that uninitialised padding.

Using memcmp to implement equality on objects seems a bit dangerous, because
it means the comparison result will be undefined any time the compiler introduces
an alignment hole for whatever reason.
Patch for discussion.  Note that this includes |double scaleDownBy| as
one of the fields participating in the comparison.  I want to double
check with y'all about whether that is really intended, given that equality
comparisons on floating point numbers are potentially problematic.

This patch stops valgrind complaining, at least.
(In reply to Julian Seward [:jseward] from comment #2)
> My best guess about this is as follows.  275908:3baf48e3f758 (bug 1192390)
> introduced the class EncodingConstraints as it stands now, except without
> "double scaleDownBy".  It also introduced a memcmp-based operator==, which
> strikes me as rather dubious, but it happened to work, because the vtbl
> pointer and all the uint32_t fields are packed back-to-back with no holes.
> 

   Well, there's no vtbl pointer since there are no virtual functions.

> Some time later, 285389:607884f102a7 (bug 1244913) added "double
> scaleDownBy".
> This requires 64-bit alignment, whereas all the other 9 fields are uint32_t
> and
> require only 32-bit alignment.  The implication is that the compiler is
> forced
> to put a 32-bit padding field after "uint32_t maxDpb" to create the correct
> alignment, and memcmp now falls across that uninitialised padding.
> 
> Using memcmp to implement equality on objects seems a bit dangerous, because
> it means the comparison result will be undefined any time the compiler
> introduces
> an alignment hole for whatever reason.

   Correct. It seems probable that this is what happened.
(In reply to Byron Campen [:bwc] from comment #4)
>    Well, there's no vtbl pointer since there are no virtual functions.

Ok, that just makes the analysis simpler.  So what we have is 9 x uint32_t,
a 32-bit padding hole, and a double.
After looking at what uses this comparator, I'm happy with your patch. Go ahead and update the commit message and ask for review.
Attachment #8724725 - Flags: review?(docfaraday)
Comment on attachment 8724725 [details] [diff] [review]
bug1252073-1.cset

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

Just make sure to update the commit message.
Attachment #8724725 - Flags: review?(docfaraday) → review+
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Assignee: nobody → jseward
https://hg.mozilla.org/mozilla-central/rev/11e3aa6fb74d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.