1192058 - Undefined behavior in CompositorD3D11::EndFrame is latent bug

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[q1]

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 .

Comment 1

[q1]

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.

Comment 2

[Jamie Nicol [:jnicol]]

Attachment: Patch v1

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().

Comment 3

[Benoit Girard (:BenWa)]

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.

Comment 4

[Jamie Nicol [:jnicol]]

Attachment: Patch v2

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?

Comment 5

[Jamie Nicol [:jnicol]]

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.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8647474 [details] [diff] [review]
Patch v2

As a sec-other, this doesn't need sec-approval. Just check it into trunk.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing.

Comment 8

[Jamie Nicol [:jnicol]]

Attachment: Patch v3

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?

Comment 9

[Benoit Girard (:BenWa)]

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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2d43bdbaee

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e2d43bdbaee