Closed Bug 1267272 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: SVG, defect, critical)

45 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed
firefox-esr38 --- wontfix
firefox-esr45 --- wontfix

People

(Reporter: abbGZcvu_bugzilla.mozilla.org, Assigned: longsonr)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files, 2 obsolete files)

Attached image Repro (obsolete) —
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
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.
(offset 0x6 from the start of the function code that is, in other words at the very start of the function).
Doesn't crash for me and mLengthAttributes can't be null as it's an array member of SVGSVGElement
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)
Attached image image.svg
Attached image repro.svg
Attachment #8744926 - Attachment is obsolete: true
You really do need it to point to something that exists to cause a crash.
Attached patch 1267272.txt (obsolete) — Splinter Review
Assignee: nobody → longsonr
Attachment #8745729 - Flags: review?(seth)
Attached patch 1267272.txtSplinter 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]
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+
https://hg.mozilla.org/mozilla-central/rev/01b755909dc1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1279124
Can you uplift the fix to Beta 48 to fix crash?
Flags: needinfo?(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
Flags: needinfo?(longsonr)
Attachment #8745733 - Flags: approval-mozilla-beta?
Crash Signature: [@ mozilla::dom::SVGSVGElement::GetIntrinsicHeight]
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
You need to log in before you can comment on or make changes to this bug.