If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

NULL pointer mozilla::dom::SVGSVGElement::GetIntrinsicHeight

RESOLVED FIXED in Firefox 48

Status

()

Core
SVG
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: SkyLined, Assigned: Robert Longson)

Tracking

({crash, testcase})

45 Branch
mozilla49
crash, testcase
Points:
---

Firefox Tracking Flags

(firefox47 wontfix, firefox48 fixed, firefox49 fixed, firefox-esr38 wontfix, firefox-esr45 wontfix)

Details

(crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
Created attachment 8744926 [details]
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")
Component: DOM: Core & HTML → SVG
(Reporter)

Comment 1

a year ago
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.
(Reporter)

Comment 2

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

Comment 3

a year ago
Doesn't crash for me and mLengthAttributes can't be null as it's an array member of SVGSVGElement
(Reporter)

Comment 4

a year ago
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)
(Assignee)

Comment 5

a year ago
Created attachment 8745155 [details]
image.svg
(Assignee)

Comment 6

a year ago
Created attachment 8745156 [details]
repro.svg
(Assignee)

Updated

a year ago
Attachment #8744926 - Attachment is obsolete: true
(Assignee)

Comment 7

a year ago
You really do need it to point to something that exists to cause a crash.
(Assignee)

Comment 8

a year ago
Created attachment 8745729 [details] [diff] [review]
1267272.txt
Assignee: nobody → longsonr
Attachment #8745729 - Flags: review?(seth)
(Assignee)

Comment 9

a year ago
Created attachment 8745733 [details] [diff] [review]
1267272.txt

wrong extension in previous patch
Attachment #8745729 - Attachment is obsolete: true
Attachment #8745729 - Flags: review?(seth)
Attachment #8745733 - Flags: review?(seth)
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.
Attachment #8745733 - Flags: review?(seth) → review+
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/01b755909dc1
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Updated

a year ago
Blocks: 1279124

Comment 14

a year ago
Can you uplift the fix to Beta 48 to fix crash?
Flags: needinfo?(longsonr)
(Assignee)

Comment 15

a year ago
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
Flags: needinfo?(longsonr)
Attachment #8745733 - Flags: approval-mozilla-beta?
Crash Signature: [@ mozilla::dom::SVGSVGElement::GetIntrinsicHeight]
status-firefox47: --- → wontfix
status-firefox48: --- → affected
status-firefox-esr38: --- → wontfix
status-firefox-esr45: --- → wontfix
Comment on attachment 8745733 [details] [diff] [review]
1267272.txt

Probably a safe fix, has tests, etc
Should be in 48 beta 2
Attachment #8745733 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1279124

Comment 18

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d5072d52f4ca
status-firefox48: affected → fixed
You need to log in before you can comment on or make changes to this bug.