Closed Bug 1348564 Opened 3 years ago Closed 3 years ago

Null deref crash [@ nsSVGUtils::GetBBox]

Categories

(Core :: SVG, defect, critical)

55 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: truber, Assigned: u459114)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(4 files, 1 obsolete file)

Attached image testcases.svg
The attached test case causes a null dereference crash in mozilla-central rev 23a4b7430dd7.

==30849==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f4a3f7f8126 bp 0x7ffd3bf886f0 sp 0x7ffd3bf88100 T0)
==30849==The signal is caused by a READ memory access.
==30849==Hint: address points to the zero page.
    #0 0x7f4a3f7f8125 in nsSVGUtils::GetBBox(nsIFrame*, unsigned int) /home/worker/workspace/build/src/layout/svg/nsSVGUtils.cpp:1143:17
    #1 0x7f4a3f0ce662 in nsStyleTransformMatrix::TransformReferenceBox::EnsureDimensionsAreCached() /home/worker/workspace/build/src/layout/style/nsStyleTransformMatrix.cpp:65:22
    #2 0x7f4a3fb02dcd in nsStyleTransformMatrix::TransformReferenceBox::X() /home/worker/workspace/build/src/layout/style/nsStyleTransformMatrix.h:106:7
    #3 0x7f4a3fab4cd7 in nsDisplayTransform::GetDeltaToTransformOrigin(nsIFrame const*, float, nsRect const*) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:6629:33
    #4 0x7f4a3fb05580 in FrameTransformProperties /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:6737:24
    #5 0x7f4a3fb05580 in nsDisplayTransform::GetResultingTransformMatrix(nsIFrame const*, nsPoint const&, float, unsigned int, nsRect const*) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:6763
    #6 0x7f4a3fb0c8e8 in nsDisplayTransform::TransformRect(nsRect const&, nsIFrame const*, nsRect const*) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:7497:6
    #7 0x7f4a3f48b631 in UnionBorderBoxes(nsIFrame*, bool, bool&, nsSize const*, nsOverflowAreas const*) /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:8617:9
    #8 0x7f4a3f48bf30 in UnionBorderBoxes(nsIFrame*, bool, bool&, nsSize const*, nsOverflowAreas const*) /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:8673:26
    #9 0x7f4a3f45d44a in ComputeAndIncludeOutlineArea /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:8751:17
    #10 0x7f4a3f45d44a in nsIFrame::FinishAndStoreOverflow(nsOverflowAreas&, nsSize, nsSize*) /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:8894
    #11 0x7f4a3f77cb73 in nsSVGDisplayContainerFrame::ReflowSVG() /home/worker/workspace/build/src/layout/svg/nsSVGContainerFrame.cpp:402:3
    #12 0x7f4a3f77ca19 in nsSVGDisplayContainerFrame::ReflowSVG() /home/worker/workspace/build/src/layout/svg/nsSVGContainerFrame.cpp:361:17
    #13 0x7f4a3f7e246a in nsSVGOuterSVGFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/svg/nsSVGOuterSVGFrame.cpp:452:14
Flags: in-testsuite?
Blocks: 1320036
Keywords: regression
Version: Trunk → 55 Branch
Assignee: nobody → cku
Attachment #8849506 - Flags: review?(jwatt)
Attachment #8849507 - Flags: review?(jwatt)
Attachment #8849531 - Flags: review?(jwatt)
Comment on attachment 8849506 [details]
Bug 1348564 - Part 1. Fix crash in nsSVGUtils::GetBBox.

https://reviewboard.mozilla.org/r/122300/#review124488

Oops, sorry for giving you broken code here originally.

::: commit-message-05bfa:1
(Diff revision 2)
> +Bug 1348564 - Part 1. Correct the condition of entering if statement.

The first line of your comment should be a high level summary of what it is you're doing, and helpful to someone looking at the message in the context of processing all commits that are landing. There are many 'if' statements, so this first line isn't so helpful. How about "Fix crash in nsSVGUtils::GetBBox"?

::: commit-message-05bfa:3
(Diff revision 2)
> +A nsSVGGradientFrame frame does have SVG layout but no nsISVGChildFrame exported
> +from it. Prevent this kind of frame entering this if statement.

Probably make this:

This change fixes the code so that it does not assume that frames with the NS_FRAME_SVG_LAYOUT bit set implement nsISVGChildFrame. This assumption is incorrect since there are many SVG frame types that do not inherit nsISVGChildFrame (such as nsSVGGradientFrame).
Attachment #8849506 - Flags: review?(jwatt) → review+
Comment on attachment 8849507 [details]
Bug 1348564 - Part 2. Crash test.

https://reviewboard.mozilla.org/r/122302/#review124492
Attachment #8849507 - Flags: review?(jwatt) → review+
Comment on attachment 8849531 [details]
Bug 1348564 - Part 3. Fix assertion in nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame.

https://reviewboard.mozilla.org/r/122326/#review124494

::: layout/svg/nsSVGIntegrationUtils.cpp:198
(Diff revision 2)
>  nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(nsIFrame* aNonSVGFrame)
>  {
> -  NS_ASSERTION(!(aNonSVGFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT),
> -               "Frames with SVG layout should not get here");
> +#ifdef DEBUG
> +  const bool hasSVGLayout = aNonSVGFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT;
> +  nsISVGChildFrame* svg = do_QueryFrame(aNonSVGFrame);
> +  NS_ASSERTION(!hasSVGLayout || !svg,

I believe that the frames that implement nsISVGChildFrame are a strict subset of the frames that have the NS_FRAME_SVG_LAYOUT bit set on them. In that case your new code is equivalent to simply asserting |!svg|, which is not correct. That would allow an nsGradientFrame to be passed to this function which should never happen.
Attachment #8849531 - Flags: review?(jwatt)
Comment on attachment 8849506 [details]
Bug 1348564 - Part 1. Fix crash in nsSVGUtils::GetBBox.

https://reviewboard.mozilla.org/r/122300/#review124500
Attachment #8849506 - Flags: review+ → review-
Comment on attachment 8849531 [details]
Bug 1348564 - Part 3. Fix assertion in nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame.

https://reviewboard.mozilla.org/r/122326/#review124494

> I believe that the frames that implement nsISVGChildFrame are a strict subset of the frames that have the NS_FRAME_SVG_LAYOUT bit set on them. In that case your new code is equivalent to simply asserting |!svg|, which is not correct. That would allow an nsGradientFrame to be passed to this function which should never happen.

Set A, Frames that have the NS_FRAME_SVG_LAYOUT = all-svg-frames, except nsSVGOuterFrame.
Set B, Frames that implement nsISVGChildFrame = some-svg-frames, nsSVGOuterFrame is included.

Set A does not contains Set B becasue of nsSVGOuterFrame.
So the assertion in the patch is eqaul to 
  NA_ASSERTION(!svg || aNonSVGFrame->GetType() == nsGkAtoms::svgOuterSVGFrame);

And, in nsSVGUtils::GetBBox, we actually want to pass nsGradientFrame down to nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame, does't it? nsGradientFrame is not inherited from nsISVGChildFrame, nsSVGUtils::GetBBox can only evalute bbox of a frame export nsISVGChildFrame interface.
Comment on attachment 8849506 [details]
Bug 1348564 - Part 1. Fix crash in nsSVGUtils::GetBBox.

https://reviewboard.mozilla.org/r/122300/#review124542

::: layout/svg/nsSVGUtils.cpp:1103
(Diff revision 2)
>    gfxRect bbox;
>    nsISVGChildFrame *svg = do_QueryFrame(aFrame);
>    const bool hasSVGLayout = aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT;
> -  if (hasSVGLayout || aFrame->IsSVGText() ||
> +  if ((svg && hasSVGLayout) || aFrame->IsSVGText() ||
>        // if we evaluate the following, |svg| can only be an outer-<svg> or null
>        (svg && !(aFlags & eUseFrameBoundsForOuterSVG))) {

This code change will cause us to skip down to the nsSVGIntegrationUtils::GetBBox call for things like nsSVGGradientFrame, which we don't want to do. That function is set up to handle HTML elements, and could well misbehave when it ends up processing nsSVGGradientFrame etc.
Comment on attachment 8849506 [details]
Bug 1348564 - Part 1. Fix crash in nsSVGUtils::GetBBox.

https://reviewboard.mozilla.org/r/122300/#review124544

I think what we need here is something like the following:

  gfxRect bbox;

  // First we figure out if we have an SVG frame that we want to get the bbox
  // from in the normal way by obtaining the union of all its descendants'
  // bboxes.

  nsISVGChildFrame* svg = nullptr;

  if (aFrame->IsSVGText()) {
    // It is possible to apply a gradient, pattern, clipping path, mask or
    // filter to text.  When one of these is applied to text the bbox is the
    // bbox for the entire ancestor <text> element in all cases.
    nsIFrame* ancestor = GetFirstNonAAncestorFrame(aFrame);
    if (ancestor && ancestor->IsSVGText()) {
      while (ancestor->GetType() != nsGkAtoms::svgTextFrame) {
        ancestor = ancestor->GetParent();
      }
    }
    svg = do_QueryFrame(ancestor);
    MOZ_ASSERT(svg, "Frames for SVG text must be wrapped by nsSVGTextFrame");
  } else {
    const bool hasSVGLayout = aFrame->GetStateBits() & NS_FRAME_SVG_LAYOUT;
    svg = do_QueryFrame(aFrame);
    if (hasSVGLayout && !svg) {
      // An SVG frame, but not one that can be displayed directly (for
      // example, nsGradientFrame). These can't contribute to the bbox.
      return bbox;
    }
    const bool isOuterSVG = svg && !hasSVGLayout;
    MOZ_ASSERT(aFrame->GetType() == nsGkAtoms::svgOuterSVGFrame);
    if (isOuterSVG && (aFlags & eUseFrameBoundsForOuterSVG)) {
      svg = nullptr; // don't use the normal svg bbox path
    }
  }

  if (svg) {
    < delete the old content down to the line: >
    nsIContent* content = aFrame->GetContent();
Comment on attachment 8849531 [details]
Bug 1348564 - Part 3. Fix assertion in nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame.

https://reviewboard.mozilla.org/r/122326/#review124548

See comment 14 - we shouldn't be getting here with SVG frames at all, so we don't want to make this change.
Attachment #8849531 - Flags: review-
Comment on attachment 8849506 [details]
Bug 1348564 - Part 1. Fix crash in nsSVGUtils::GetBBox.

https://reviewboard.mozilla.org/r/122300/#review124544

I see. Sure, will re-cook this patch. Thanks for this clear suggestion.
(In reply to Jonathan Watt [:jwatt] from comment #16)
> See comment 14 - we shouldn't be getting here with SVG frames at all, so we
> don't want to make this change.

With the exception of nsSVGOuterSVGFrame which is laid out using the CSS box model rules. Maybe we should make explicit mention that this function is for elements that are laid out using the CSS box model rules in a comment above the assertion. I'd r+ a patch for that.
Attachment #8849531 - Attachment is obsolete: true
Attachment #8849886 - Flags: review?(jwatt)
Comment on attachment 8849506 [details]
Bug 1348564 - Part 1. Fix crash in nsSVGUtils::GetBBox.

https://reviewboard.mozilla.org/r/122300/#review124826

Lovely. :)

::: layout/svg/nsSVGUtils.cpp:1123
(Diff revision 3)
> +    return gfxRect();
> +  }
> +
> +  const bool isOuterSVG = svg && !hasSVGLayout;
> +  MOZ_ASSERT_IF(isOuterSVG, aFrame->GetType() == nsGkAtoms::svgOuterSVGFrame);
> +  if ((isOuterSVG && (aFlags & eUseFrameBoundsForOuterSVG)) || !svg) {

To make it easier to see that we're returning if !svg, put that at the start of the expression. And maybe put the other || operand on a new line.
Attachment #8849506 - Flags: review?(jwatt) → review+
Oh, and note that I landed a patch to rename nsISVGChildFrame to nsSVGDisplayableFrame. That's going to conflict here I guess. Sorry about that. :/
Comment on attachment 8849886 [details]
Bug 1348564 - Part 3. More comment in nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame.

https://reviewboard.mozilla.org/r/122626/#review124828

Thanks for adding this.

::: layout/svg/nsSVGIntegrationUtils.cpp:195
(Diff revision 1)
>  }
>  
>  gfxRect
>  nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame(nsIFrame* aNonSVGFrame)
>  {
> +  // Except nsSVGOuterSVGFrame, we shouldn't be getting here with SVG frames

Make that "Except for nsSVGOuterSVGFrame..."
Attachment #8849886 - Flags: review?(jwatt) → review+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/851eab30794e
Part 1. Fix crash in nsSVGUtils::GetBBox. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/49a671b7db47
Part 2. Crash test. r=jwatt
https://hg.mozilla.org/integration/autoland/rev/8786f14f2da9
Part 3. More comment in nsSVGIntegrationUtils::GetSVGBBoxForNonSVGFrame. r=jwatt
You need to log in before you can comment on or make changes to this bug.