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

RESOLVED FIXED in Firefox 48



3 years ago
3 years ago


(Reporter:, Assigned: longsonr)


({crash, testcase})

45 Branch
crash, testcase

Firefox Tracking Flags

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


(crash signature)


(3 attachments, 2 obsolete attachments)



3 years ago
Created attachment 8744926 [details]


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

image.svg is an invalid svg image (e.g. a file containing "X")
Component: DOM: Core & HTML → SVG

Comment 1

3 years 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:

  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

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

Comment 3

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

Comment 4

3 years 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)

Comment 5

3 years ago
Created attachment 8745155 [details]

Comment 6

3 years ago
Created attachment 8745156 [details]


3 years ago
Attachment #8744926 - Attachment is obsolete: true

Comment 7

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

Comment 8

3 years ago
Created attachment 8745729 [details] [diff] [review]
Assignee: nobody → longsonr
Attachment #8745729 - Flags: review?(seth)

Comment 9

3 years ago
Created attachment 8745733 [details] [diff] [review]

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]

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+

Comment 13

3 years ago
Last Resolved: 3 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49


3 years ago
Blocks: 1279124

Comment 14

3 years ago
Can you uplift the fix to Beta 48 to fix crash?
Flags: needinfo?(longsonr)

Comment 15

3 years ago
Comment on attachment 8745733 [details] [diff] [review]

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]

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

3 years ago
status-firefox48: affected → fixed
You need to log in before you can comment on or make changes to this bug.