Closed Bug 1141636 Opened 5 years ago Closed 5 years ago

Dubious loop test in nsSVGElement::GetAnimatedLengthListValues


(Core :: SVG, defect)

Not set



Tracking Status
firefox39 --- fixed


(Reporter: jseward, Assigned: longsonr)



(2 files, 1 obsolete file)

dom/svg/nsSVGElement.cpp, function nsSVGElement::GetAnimatedLengthListValues
has a loop

1718   while (list && i < info.mLengthListCount) {
1719     list->Init(&(info.mLengthLists[i].GetAnimValue()),
                    this, info.mLengthListInfo[i].mAxis);
1720     ++i;
1721     list = va_arg(args, SVGUserUnitList*);
1722   }

that writes values into its varargs (out) parameters.  The last
valid iteration of the loop happens when i == info.mLengthListCount-1.

At the end of this iteration, |args| is undefined, since we've
used up all the varargs, and so |list| contains an unknown value
which -- in some 32 bit build x86 builds -- is garbage picked up
from the stack.  We then attempt another iteration of the loop,
first testing the garbage in |list| before having
"&&  i < info.mLengthListCount" stop the loop once and for all.

The code works I think because "undefined && false" == false,
but it doesn't seem good to check |list| when it's undefined -- it
may be that this takes us into the "undefined behavior" category
of C++, and for sure it confuses Valgrind.
Test is dom/base/test/csp/test_CSP_inlinestyle.html

I ran this:

killall xpcshell ; DISPLAY=:1.0 G_SLICE=always-malloc
--debugger-args="--fair-sched=yes --smc-check=all-non-file
--error-limit=no --trace-children=yes --child-silent-after-fork=yes
--num-transtab-sectors=24 --tool=memcheck --freelist-vol=500000000
--redzone-size=256 --gen-suppressions=no
--px-file-backed=unwindregs-at-mem-access --stats=yes
--partial-loads-ok=yes --show-mismatched-frees=no
--read-inline-info=yes --fullpath-after=-CURR/ --num-callers=32
--track-origins=no --show-leak-kinds=definite"
dom/base/test/csp/test_CSP_inlinestyle.html 2>&1 | tee spew-45-mc
Attached patch A possible fix (obsolete) — Splinter Review
Or maybe simply get rid of the varargs calling convention for
nsSVGElement::GetAnimatedLengthListValues ?  It's only ever called
from one place and with a fixed number (4) args, so the varargs
seems somewhat overkill.
Yeah, given that there is only one remaining caller of this function, removing its vararg-ness sounds good.

It would make the function a bit less generic, though, so I think it would make sense to move it down to SVGTextPositioningElement, and call the SVGUserUnitList* arguments aX, aY, aDX, aDY.
The callers final argument should be nullptr.
Attached patch text.txtSplinter Review
Assignee: nobody → longsonr
Attachment #8575395 - Attachment is obsolete: true
Attachment #8576500 - Flags: review?(cam)
Attachment #8576500 - Flags: feedback?(jseward)
Comment on attachment 8576500 [details] [diff] [review]

Review of attachment 8576500 [details] [diff] [review]:

That works.  What do you think about de-vararging the method?
Attachment #8576500 - Flags: review?(cam) → review+
It is one of a number of vararg methods for the various different attribute types. It's more efficient than obtaining each attribute individually which would be the alternative.
Wr is still failing and there's no SVG code in Wr anyway.
(In reply to Robert Longson from comment #11)
> Wr is still failing and there's no SVG code in Wr anyway.

yeah will checkin this again, sorry for backout
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attachment #8576500 - Flags: feedback?(jseward)
You need to log in before you can comment on or make changes to this bug.