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

Status: RESOLVED FIXED

Description

[Julian Seward [:jseward]]

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

Comment 1

[Julian Seward [:jseward]]

Created attachment 8724715 [details]
Valgrind complaint

Comment 2

[Julian Seward [:jseward]]

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.

Comment 3

[Julian Seward [:jseward]]

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

[Byron Campen [:bwc]]

(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.

Comment 5

[Julian Seward [:jseward]]

(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

[Byron Campen [:bwc]]

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

Comment 7

[Byron Campen [:bwc]]

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.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11e3aa6fb74d

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11e3aa6fb74d