Closed
Bug 1252073
Opened 6 years ago
Closed 6 years ago
Uninitialised value uses in mozilla::EncodingConstraints::operator==
Categories
(Core :: WebRTC: Signaling, defect, P1)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: jseward, Assigned: jseward)
Details
Attachments
(2 files)
4.47 KB,
text/plain
|
Details | |
1.02 KB,
patch
|
bwc
:
review+
|
Details | Diff | Splinter Review |
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•6 years ago
|
||
Assignee | ||
Comment 2•6 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•6 years ago
|
||
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•6 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•6 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•6 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•6 years ago
|
Attachment #8724725 -
Flags: review?(docfaraday)
Comment 7•6 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•6 years ago
|
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
Updated•6 years ago
|
Assignee: nobody → jseward
Comment 9•6 years ago
|
||
bugherder |
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.
Description
•