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

RESOLVED FIXED in Firefox 47

Status

()

Core
WebRTC: Signaling
P1
normal
Rank:
15
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

Trunk
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
Created attachment 8724715 [details]
Valgrind complaint
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8724725 [details] [diff] [review]
bug1252073-1.cset

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.

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

2 years ago
(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.

Comment 6

2 years ago
After looking at what uses this comparator, I'm happy with your patch. Go ahead and update the commit message and ask for review.
(Assignee)

Updated

2 years ago
Attachment #8724725 - Flags: review?(docfaraday)

Comment 7

2 years ago
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+

Updated

2 years ago
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Assignee: nobody → jseward

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/11e3aa6fb74d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.