Closed Bug 1192058 Opened 6 years ago Closed 6 years ago

Undefined behavior in CompositorD3D11::EndFrame is latent bug

Categories

(Core :: Graphics, defect)

39 Branch
Unspecified
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: q1, Assigned: jnicol)

Details

(Keywords: sec-other, Whiteboard: [post-critsmash-triage][adv-main43-])

Attachments

(1 file, 2 obsolete files)

CompositorD3D11::EndFrame (\gfx\layers\d3d11\CompositorD3D11.cpp) invokes undefined behavior by dereferencing a past-the-end iterator to an std::vector. At present, this seems to have no ill effects, but nothing guarantees that that situation will continue:

1150:      params.DirtyRectsCount = mInvalidRegion.GetNumRects();
1151:      std::vector<RECT> rects;
1152:      rects.reserve(params.DirtyRectsCount);
1153:
1154:      nsIntRegionRectIterator iter(mInvalidRegion);
1155:      const nsIntRect* r;
1156:      uint32_t i = 0;
1157:      while ((r = iter.Next()) != nullptr) {
1158:        RECT rect;
1159:        rect.left = r->x;
1160:        rect.top = r->y;
1161         rect.bottom = r->YMost();
1162:        rect.right = r->XMost();
1163:
1164:        rects.push_back(rect);
1165:      }
1166:
1167:      params.pDirtyRects = &rects.front();
1168:      chain->Present1(presentInterval, mDisableSequenceForNextFrame ? DXGI_PRESENT_DO_NOT_SEQUENCE : 0, &params);

The problem is that params.DirtyRectsCount sometimes == 0. This occurs when, for example, creating a new window. (You can verify this by setting a conditional breakpoint on line 1150, then pressing ctrl/n). Control passes to line 1167, which takes &rects.front(). But rects.front() is undefined here, because it's the same as *rects.begin(), and rects.begin()==rects.end().

In the STL implementation that gets built into FF 39.0 when building with VS2012, the expression |&rects.front()| happens to have the value NULL, which is coincidentally appropriate for the call to IDXGISwapChain1::Present1 on line 1168. However, MS specifically calls out vector::front() as undefined for an empty vector. https://msdn.microsoft.com/en-us/library/0z70c7a5.aspx .
Assignee: nobody → jnicol
OS: Unspecified → Windows
Just to clarify, |container.front()| for an empty container is undefined behavior in all STL implementations, since it dereferences the non-dereferenceable iterator |container.end()|. I used the example of Windows because I've built that version of FF.
Keywords: sec-other
Attached patch Patch v1 (obsolete) — Splinter Review
Only set params.pDirtyRects if the vector is not empty. If the vector is empty then params.pDirtyRects will be null due to the PodZero(&params) call above.

Also might as well use vector::data() instead of vector::front().
Attachment #8647020 - Flags: review?(bgirard)
Comment on attachment 8647020 [details] [diff] [review]
Patch v1

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +1108,5 @@
>        }
>  
> +      if (!rects.empty()) {
> +        params.pDirtyRects = rects.data();
> +      }

Looks good, personally I would prefer to have |else { params.pDirtyRects = nullptr; }|. It would be easy to forget this if we removed the PodZero call above.
Attachment #8647020 - Flags: review?(bgirard) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
Explicitly set to nullptr as per Benoit's suggestion.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6df50c3803a

I'm not sure what the next steps are here. Usually I'd set checkin-needed but Milan said this needs a security review first? Is there a flag I set for that?
Attachment #8647020 - Attachment is obsolete: true
Comment on attachment 8647474 [details] [diff] [review]
Patch v2

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Not easily

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No

> Which older supported branches are affected by this flaw?

All

> If not all supported branches, which bug introduced the flaw?

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They will be the same patch, possibly need to be adjusted for line numbers, but that will be trivial.

> How likely is this patch to cause regressions; how much testing does it need?

Not likely at all.
Attachment #8647474 - Flags: sec-approval?
Comment on attachment 8647474 [details] [diff] [review]
Patch v2

As a sec-other, this doesn't need sec-approval. Just check it into trunk.
Attachment #8647474 - Flags: sec-approval?
Keywords: checkin-needed
Needs rebasing.
Keywords: checkin-needed
Attached patch Patch v3Splinter Review
The commit that caused the conflict with this also fixed the problem - we no longer even use std::vector here.

We could still explicity set pDirtyRects to nullptr but I'm not sure this is necessary. The docs say "You can set this member to NULL if DirtyRectsCount is 0", not "you must...". - https://msdn.microsoft.com/en-us/library/windows/desktop/hh404522(v=vs.85).aspx

Benoit: what do you think? should we commit or abandon this?
Attachment #8649281 - Flags: review?(bgirard)
Attachment #8647474 - Attachment is obsolete: true
Comment on attachment 8649281 [details] [diff] [review]
Patch v3

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

IMO I'd rather have this be nullptr than stack address. You've already written the patch anyways. But like you said I agree that it's probably not really needed.
Attachment #8649281 - Flags: review?(bgirard) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5e2d43bdbaee
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main43-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.