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 755514 - Potential integer overflow in clutter_deform_effect_init_arrays
Potential integer overflow in clutter_deform_effect_init_arrays
Status: RESOLVED OBSOLETE
Product: clutter
Classification: Platform
Component: ClutterEffect
git master
Other Linux
: Normal normal
: ---
Assigned To: clutter-maint
clutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-09-24 09:20 UTC by Florian Weimer
Modified: 2021-06-10 11:32 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Florian Weimer 2015-09-24 09:20:48 UTC
This code should check that the multiplications cannot overflow:

  priv->n_vertices = (priv->x_tiles + 1) * (priv->y_tiles + 1);

  priv->buffer =
    cogl_attribute_buffer_new (ctx,
                               sizeof (CoglVertexP3T2C4) *
                               priv->n_vertices,
                               NULL);

This is related to a similar bug in libmx reported here:

https://bugzilla.redhat.com/show_bug.cgi?id=918974
Comment 1 Emmanuele Bassi (:ebassi) 2015-09-24 09:45:36 UTC
This would be a use case for the API in bug 503096, but for the time being we can solve it locally.

The size is stored as an unsigned integer in CoglBuffer, so we need something like:

  guint a, b;
  if ((G_MAXUINT - priv->x_tiles) < 1)
    a = G_MAXUINT;
  else
    a = priv->x_tiles + 1;
  if ((G_MAXUINT - priv->y_tiles) < 1)
    b = G_MAXUINT;
  else
    b = priv->y_tiles + 1;

  if (b && (G_MAXUINT / b) < a)
    priv->n_tiles = G_MAXUINT;
  else
    priv->n_tiles = a * b;

This should take care of overflowing both arithmetic operations.
Comment 2 Emmanuele Bassi (:ebassi) 2015-09-24 09:48:22 UTC
There's also a potential overflow in above that one:

  int n_indices = ((2 + 2 * priv->x_tiles)
                * priv->y_tiles
                + (priv->y_tiles - 1));

  guint16 *static_indices = g_new (guint16, n_indices);

which will require some careful unrolling.
Comment 3 Florian Weimer 2015-09-24 09:49:18 UTC
(In reply to Emmanuele Bassi (:ebassi) from comment #1)

> The size is stored as an unsigned integer in CoglBuffer, so we need
> something like:

There is another multiplication: sizeof (CoglVertexP3T2C4) * priv->n_vertices

It could be more straightforward to perform the arithmetic in unsigned long long (and it would avoid the division).
Comment 4 Emmanuele Bassi (:ebassi) 2015-09-24 09:54:21 UTC
(In reply to Florian Weimer from comment #3)
> (In reply to Emmanuele Bassi (:ebassi) from comment #1)
> 
> > The size is stored as an unsigned integer in CoglBuffer, so we need
> > something like:
> 
> There is another multiplication: sizeof (CoglVertexP3T2C4) * priv->n_vertices

True.
 
> It could be more straightforward to perform the arithmetic in unsigned long
> long (and it would avoid the division).

Even if we did use a wider type, we'd have to pass it down into Cogl, which would then store it as an unsigned int, so we'd need to clamp it in any case.
Comment 5 André Klapper 2021-06-10 11:32:34 UTC
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version of clutter, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a ticket at
  https://gitlab.gnome.org/GNOME/clutter/-/issues/

Thank you for your understanding and your help.