1267272 - NULL pointer mozilla::dom::SVGSVGElement::GetIntrinsicHeight

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[SkyLined]

Attachment: Repro

Repro:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <filter id="f">
  	<feImage xlink:href="image.svg"/>
  </filter>
  <rect filter='url(#f)' width="1" height="1"/>
</svg>

image.svg is an invalid svg image (e.g. a file containing "X")

Comment 1

[SkyLined]

Source code as reported by symbols does not match up exactly with online source, but it does help pinpoint the source for the function:

https://dxr.mozilla.org/mozilla-central/source/dom/svg/SVGSVGElement.cpp#1179

int32_t
SVGSVGElement::GetIntrinsicHeight()
{
  if (mLengthAttributes[ATTR_HEIGHT].IsPercentage()) {
    return -1;
  }
  // Passing |this| as a SVGSVGElement* invokes the variant of GetAnimValue
  // that uses the passed argument as the context, but that's fine since we
  // know the length isn't a percentage so the context won't be used (and we
  // need to pass the element to be able to resolve em/ex units).
  float height = mLengthAttributes[ATTR_HEIGHT].GetAnimValue(this);
  return nsSVGUtils::ClampToInt(height);
}

Given that the NULL pointer happens at offset 0x6, I am expecting "mLengthAttributes" to be NULL in this case, but I am speculating.

Comment 2

[SkyLined]

(offset 0x6 from the start of the function code that is, in other words at the very start of the function).

Comment 3

[Robert Longson [:longsonr]]

Doesn't crash for me and mLengthAttributes can't be null as it's an array member of SVGSVGElement

Comment 4

[SkyLined]

I'm using the 45 release branch and loading from a webserver (rather than a local file).

You are correct, after looking at the disassembly I think the mLengthAttributes[ATTR_HEIGHT] is NULL instead. In hindsight, the source suggests this too.

xul!mozilla::dom::SVGSVGElement::GetIntrinsicHeight [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\dom\svg\svgsvgelement.cpp @ 1178]:
58abde02 55              push    ebp
58abde03 8bec            mov     ebp,esp
58abde05 83ec10          sub     esp,10h
58abde08 80b9a400000002  cmp     byte ptr [ecx+0A4h],2           ⇐ Crash here
58abde0f 7447            je      xul!mozilla::dom::SVGSVGElement::GetIntrinsicHeight+0x56 (58abde58)

Comment 5

[Robert Longson [:longsonr]]

Attachment: image.svg

Comment 6

[Robert Longson [:longsonr]]

Attachment: repro.svg

Comment 7

[Robert Longson [:longsonr]]

You really do need it to point to something that exists to cause a crash.

Comment 8

[Robert Longson [:longsonr]]

Attachment: 1267272.txt

Comment 9

[Robert Longson [:longsonr]]

Attachment: 1267272.txt

wrong extension in previous patch

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8745733 [details] [diff] [review]
1267272.txt

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

Thanks Robert! Looks good.

::: image/VectorImage.cpp
@@ +687,5 @@
>  VectorImage::GetFrame(uint32_t aWhichFrame, uint32_t aFlags)
>  {
> +  if (mError) {
> +    return nullptr;
> +  }

Please add a blank line after this |if| block.

Comment 11

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6883fd793ab30995b9606e20b470183f2949a655

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01b755909dc1

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01b755909dc1

Comment 14

[Alice0775 White]

Can you uplift the fix to Beta 48 to fix crash?

Comment 15

[Robert Longson [:longsonr]]

Comment on attachment 8745733 [details] [diff] [review]
1267272.txt

Approval Request Comment
[Feature/regressing bug #]: unknown, possibly a very old bug.
[User impact if declined]: Firefox crashes when encountering the invalid image
[Describe test coverage new/current, TreeHerder]: includes a crashtest
[Risks and why]: almost nonr, it's just a null check
[String/UUID change made/needed]: none

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8745733 [details] [diff] [review]
1267272.txt

Probably a safe fix, has tests, etc
Should be in 48 beta 2

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d5072d52f4ca