550211 - nsWindow::SetWindowClipRegion: undefined value use in pixman_region32_intersect

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Julian Seward [:jseward]]

nsWindow::SetWindowClipRegion in widget/src/gtk2/nsWindow.cpp can
call pixman_region32_equal passing it uninitialised stack memory.
Some of the resulting errors are shown below.

HOW TO REPRO: run layout/generic/test/test_plugin_clipping.xhtml
on Memcheck as described in bug 549224.


ANALYSIS:

Line numbers are in widdget/src/gtk2/nsWindow.cpp
The uninitialised memory is created:

4713:        nsAutoRef<pixman_region32> intersectRegion;

This call is intended to fill it in:

4714:        pixman_region32_intersect(&intersectRegion,
                                       &newRegion, &existingRegion);

and the value of intersectRegion is then used here:

4717:        if (pixman_region32_equal(&intersectRegion, &existingRegion))
4718:            return;

Problem is that pixman_region32_intersect can return without writing
data to intersectRegion.  Line numbers in pixman-region.c:

1196: PIXMAN_EXPORT pixman_bool_t
1197: PREFIX (_intersect) (region_type_t *     new_reg,
1198:                      region_type_t *        reg1,
1199:                      region_type_t *        reg2)

So the out-param is "new_reg".  This is possible:

1205:    /* check for trivial reject */
1206:    if (PIXREGION_NIL (reg1) || PIXREGION_NIL (reg2) ||
1207:        !EXTENTCHECK (&reg1->extents, &reg2->extents))
1208:    {
1209:        /* Covers about 20% of all cases */
1210:        FREE_DATA (new_reg);
1211:        new_reg->extents.x2 = new_reg->extents.x1;
1212:        new_reg->extents.y2 = new_reg->extents.y1;
1213:        if (PIXREGION_NAR (reg1) || PIXREGION_NAR (reg2))
1214:        {
1215:            new_reg->data = pixman_broken_data;
1216:            return FALSE;
1217: 	     }

A return is possible at line 1216.  If (as is the case here)
new_reg->extents.x1 and .y1 are uninitialised, then on return
.x2 and .y2 will be tool.

Notice that nsWindow::SetWindowClipRegion ingores the return value of
pixman_region32_intersect and so is unable to detect that the
out-parameter has not been properly filled in.



RESULTING ERROR: this and 3 others similar

Conditional jump or move depends on uninitialised value(s)
   at 0x6234054: _moz_pixman_region32_equal (pixman-region.c:335)
   by 0x5F750D1: nsWindow::SetWindowClipRegion(nsTArray<nsIntRect> const&, int) (nsWindow.cpp:4717)
   by 0x5F75334: nsWindow::ConfigureChildren(nsTArray<nsIWidget::Configuration> const&) (nsWindow.cpp:4685)
   by 0x57655FF: nsRootPresContext::UpdatePluginGeometry(nsIFrame*) (nsPresContext.cpp:2484)
   by 0x57C2608: nsObjectFrame::CreateWidget(int, int, int) (nsObjectFrame.cpp:743)
   by 0x57C2978: nsPluginInstanceOwner::CreateWidget() (nsObjectFrame.cpp:5605)
   by 0x5E991EE: nsPluginHost::InstantiateEmbeddedPlugin(char const*, nsIURI*, nsIPluginInstanceOwner*) (nsPluginHost.cpp:2477)
   by 0x57C0EB3: nsObjectFrame::InstantiatePlugin(nsIPluginHost*, char const*, nsIURI*) (nsObjectFrame.cpp:945)
   by 0x57C6D0B: nsObjectFrame::Instantiate(char const*, nsIURI*) (nsObjectFrame.cpp:2061)
   by 0x5938E20: nsObjectLoadingContent::Instantiate(nsIObjectFrame*, nsACString_internal const&, nsIURI*) (nsObjectLoadingContent.cpp:1858)
   by 0x59391E2: nsObjectLoadingContent::TryInstantiate(nsACString_internal const&, nsIURI*) (nsObjectLoadingContent.cpp:1815)
   by 0x5939DEE: nsObjectLoadingContent::LoadObject(nsIURI*, int, nsCString const&, int) (nsObjectLoadingContent.cpp:1279)
 Uninitialised value was created by a stack allocation
   at 0x5F74FD7: nsWindow::SetWindowClipRegion(nsTArray<nsIntRect> const&, int) (nsWindow.cpp:4700)

Comment 1

[Julian Seward [:jseward]]

Attachment: a possible fix

One possible fix is simply to zero out intersectRegion.extents
before the call, which is what this patch does.  A more sophisticated
approach would be to look at the return value from pixman_region32_intersect
and do something different if the call fails.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: init pixman_region32_equal out-param [Checkin: Comment 3]

Thanks, Julien.

It seems that 2 empty pixman regions are not necessarily equal.  That's
probably a bug in pixman's _equal, but then our use of a pixman region for the
out parameter without first an _init on that region is probably not perfectly
pure and holy either.

The equal check is just an optimization, so the consequences of the false
negative are minor.

The tidiest fix I think is to use pixman_region32_init.  As with attachment
430346 [review], this means that this region will be considered equal to an empty
newRegion or existingRegion as compared here.

Comment 3

[Serge Gautherie (:sgautherie)]

Comment on attachment 430770 [details] [diff] [review]
init pixman_region32_equal out-param
[Checkin: Comment 3]


http://hg.mozilla.org/mozilla-central/rev/2796b616db94

Comment 4

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/0c152b45e0f5

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 6

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430