After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 636491 - valgrind: meta_window_shape_new (meta-window-shape.c:79)
valgrind: meta_window_shape_new (meta-window-shape.c:79)
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2010-12-04 23:59 UTC by Colin Walters
Modified: 2010-12-06 17:01 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
meta_region_iterator: Assert if we somehow get 0 rectangles (1.11 KB, patch)
2010-12-05 20:04 UTC, Colin Walters
rejected Details | Review
MetaRegionIterator: avoid reading off end of rectangles array (1.02 KB, patch)
2010-12-06 16:52 UTC, Owen Taylor
committed Details | Review

Description Colin Walters 2010-12-04 23:59:43 UTC
==8598== Conditional jump or move depends on uninitialised value(s)
==8598==    at 0x806F6BD: meta_window_shape_new (meta-window-shape.c:79)
==8598==    by 0x806CB8D: meta_window_actor_pre_paint (meta-window-actor.c:1897)
==8598==    by 0x806E276: meta_window_actor_get_paint_volume (meta-window-actor.c:708)
==8598==    by 0x46E1DDB: _clutter_actor_get_paint_volume_mutable (clutter-actor.c:11615)
==8598==    by 0x46E2AA4: _clutter_actor_finish_queue_redraw (clutter-actor.c:4973)
==8598==    by 0x4744EDA: _clutter_stage_do_update (clutter-stage.c:3295)
==8598==    by 0x472B37E: clutter_clock_dispatch (clutter-master-clock.c:384)
==8598==    by 0x4AD0C64: g_main_context_dispatch (gmain.c:2440)
==8598==    by 0x4AD5217: g_main_context_iterate (gmain.c:3091)
==8598==    by 0x4AD5756: g_main_loop_run (gmain.c:3299)
==8598==    by 0x808572B: main (main.c:722)

I don't see how this one is possible, we do:


  shape->n_rectangles = cairo_region_num_rectangles (region);

  if (shape->n_rectangles == 0)
    {
      ...
      return;
    }

And the only way meta_region_iterator_init leaves iter->line_end uninitialized is if iter->n_rectangles == 0...hmm...unless it's negative!  Which implies to me that pixman state is corrupted somehow (surely negative rectangles are invalid)?

We could add an assertion that shape->n_rectangles >= 0, or we could just return if shape->n_rectangles <= 0.

But it's worth investigating how this could happen.  The culprit to me looks like memory management of priv->shape_region in mutter-window-actor.c.
Comment 1 Colin Walters 2010-12-05 00:05:49 UTC
Probably a duplicate:

==8598== Conditional jump or move depends on uninitialised value(s)
==8598==    at 0x806FD7A: meta_make_border_region (region-utils.c:287)
==8598==    by 0x8069C51: meta_shadow_factory_get_shadow (meta-shadow-factory.c:692)
==8598==    by 0x806C729: meta_window_actor_pre_paint (meta-window-actor.c:1907)
==8598==    by 0x806E276: meta_window_actor_get_paint_volume (meta-window-actor.c:708)
==8598==    by 0x46E1DDB: _clutter_actor_get_paint_volume_mutable (clutter-actor.c:11615)
==8598==    by 0x46E2AA4: _clutter_actor_finish_queue_redraw (clutter-actor.c:4973)
==8598==    by 0x4744EDA: _clutter_stage_do_update (clutter-stage.c:3295)
==8598==    by 0x472B37E: clutter_clock_dispatch (clutter-master-clock.c:384)
==8598==    by 0x4AD0C64: g_main_context_dispatch (gmain.c:2440)
==8598==    by 0x4AD5217: g_main_context_iterate (gmain.c:3091)
==8598==    by 0x4AD5756: g_main_loop_run (gmain.c:3299)
==8598==    by 0x808572B: main (main.c:722)
Comment 2 Colin Walters 2010-12-05 20:04:26 UTC
Created attachment 175885 [details] [review]
meta_region_iterator: Assert if we somehow get 0 rectangles

Callers should be checking for this, the code doesn't handle it,
so let's assert instead of leaving memory uninitialized.
Comment 3 Colin Walters 2010-12-05 21:06:16 UTC
See also https://bugzilla.gnome.org/show_bug.cgi?id=635421
Comment 4 Owen Taylor 2010-12-06 04:50:58 UTC
Review of attachment 175885 [details] [review]:

it's perfectly valid to use meta_region_iterator on an empty region; there are just no rectangles to iterate.
Comment 5 Owen Taylor 2010-12-06 09:27:55 UTC
Thinking it might have something to do with:

void
meta_region_iterator_next (MetaRegionIterator *iter)
{
  iter->i++;
  iter->rectangle = iter->next_rectangle;
  iter->line_start = iter->line_end;

  if (iter->i < iter->n_rectangles)
    {
      cairo_region_get_rectangle (iter->region, iter->i + 1, &iter->next_rectangle);
      iter->line_end = iter->next_rectangle.y != iter->rectangle.y;
    }
  else
    {
      iter->line_end = TRUE;
    }
}

Probably should be:
 if (iter->i + 1 < iter->n_rectangles)
   {
      ...
  }
Comment 6 Colin Walters 2010-12-06 16:46:06 UTC
(In reply to comment #5)

> Probably should be:
>  if (iter->i + 1 < iter->n_rectangles)
>    {
>       ...
>   }

That sounds right, yes.  But I don't think this explains the uninitialized read, you'd just get an incorrect line_end; right?

From my trace of the code the only way that valgrind error could happen is if n_rectangles was negative.
Comment 7 Owen Taylor 2010-12-06 16:52:52 UTC
Created attachment 175935 [details] [review]
MetaRegionIterator: avoid reading off end of rectangles array

As discussed on IRC, I think this is Valgrind propagating uninitialized-ness.
Comment 8 Colin Walters 2010-12-06 16:56:46 UTC
Review of attachment 175935 [details] [review]:

Marking ACN per my understanding and IRC discussion.
Comment 9 Owen Taylor 2010-12-06 17:01:04 UTC
Attachment 175935 [details] pushed as 3f9c375 - MetaRegionIterator: avoid reading off end of rectangles array