(#91): Have a STORAGE_UNINITIALIZED for construction with all-default properties

If one does a plain

  GdkPixbuf *pixbuf = g_object_new (GDK_TYPE_PIXBUF, NULL);

Then all the construct properties use their default values.  This
means that both "pixels" and "pixel-bytes" get passed as NULL to
gdk_pixbuf_set_property().

Later, trying to get the property for "pixel-bytes" would assert,
incorrectly, because it was trying to create a GBytes from a NULL
pixels storage.

This commit adds a test for that construction case, and tests for
constructing with g_object_new() in general.

Fixes https://gitlab.gnome.org/GNOME/gdk-pixbuf/issues/91

4 files changed, 291 insertions, 97 deletions

diff --git a/gdk-pixbuf/gdk-pixbuf-private.h b/gdk-pixbuf/gdk-pixbuf-private.h
index 79ccf1531..343724c7e 100644
--- a/gdk-pixbuf/gdk-pixbuf-private.h
+++ b/gdk-pixbuf/gdk-pixbuf-private.h
@@ -50,6 +50,7 @@ typedef struct _GdkPixbufClass GdkPixbufClass;
 #define DEFAULT_FILL_COLOR 0x979899ff
 
 typedef enum {
+        STORAGE_UNINITIALIZED,
         STORAGE_PIXELS,
         STORAGE_BYTES
 } Storage;
diff --git a/gdk-pixbuf/gdk-pixbuf.c b/gdk-pixbuf/gdk-pixbuf.c
index 8e39ac416..f38ad8aa3 100644
--- a/gdk-pixbuf/gdk-pixbuf.c
+++ b/gdk-pixbuf/gdk-pixbuf.c
@@ -108,6 +108,7 @@ static void gdk_pixbuf_get_property (GObject        *object,
 				     guint           prop_id,
 				     GValue         *value,
 				     GParamSpec     *pspec);
+static void gdk_pixbuf_constructed  (GObject        *object);
 
 
 enum 
@@ -138,6 +139,7 @@ gdk_pixbuf_init (GdkPixbuf *pixbuf)
   pixbuf->n_channels = 3;
   pixbuf->bits_per_sample = 8;
   pixbuf->has_alpha = FALSE;
+  pixbuf->storage = STORAGE_UNINITIALIZED;
 }
 
 static void
@@ -150,6 +152,7 @@ gdk_pixbuf_class_init (GdkPixbufClass *klass)
         object_class->finalize = gdk_pixbuf_finalize;
         object_class->set_property = gdk_pixbuf_set_property;
         object_class->get_property = gdk_pixbuf_get_property;
+        object_class->constructed = gdk_pixbuf_constructed;
 
 #define PIXBUF_PARAM_FLAGS G_PARAM_READWRITE|G_PARAM_CONSTRUCT_ONLY|\
                            G_PARAM_EXPLICIT_NOTIFY|\
@@ -1239,69 +1242,81 @@ gdk_pixbuf_set_property (GObject         *object,
 			 const GValue    *value,
 			 GParamSpec      *pspec)
 {
-  GdkPixbuf *pixbuf = GDK_PIXBUF (object);
-  gboolean notify = TRUE;
-
-  switch (prop_id)
-          {
-          case PROP_COLORSPACE:
-                  notify = pixbuf->colorspace != g_value_get_enum (value);
-                  pixbuf->colorspace = g_value_get_enum (value);
-                  break;
-          case PROP_N_CHANNELS:
-                  notify = pixbuf->n_channels != g_value_get_int (value);
-                  pixbuf->n_channels = g_value_get_int (value);
-                  break;
-          case PROP_HAS_ALPHA:
-                  notify = pixbuf->has_alpha != g_value_get_boolean (value);
-                  pixbuf->has_alpha = g_value_get_boolean (value);
-                  break;
-          case PROP_BITS_PER_SAMPLE:
-                  notify = pixbuf->bits_per_sample != g_value_get_int (value);
-                  pixbuf->bits_per_sample = g_value_get_int (value);
-                  break;
-          case PROP_WIDTH:
-                  notify = pixbuf->width != g_value_get_int (value);
-                  pixbuf->width = g_value_get_int (value);
-                  break;
-          case PROP_HEIGHT:
-                  notify = pixbuf->height != g_value_get_int (value);
-                  pixbuf->height = g_value_get_int (value);
-                  break;
-          case PROP_ROWSTRIDE:
-                  notify = pixbuf->rowstride != g_value_get_int (value);
-                  pixbuf->rowstride = g_value_get_int (value);
-                  break;
-
-          /* The following two are a bit strange.  Both PROP_PIXELS and PROP_PIXEL_BYTES are
-           * G_PARAM_CONSTRUCT_ONLY properties, which means that GObject will generate default
-           * values for any missing one and call us for *both*.  So, we need to check whether the
-           * passed value is not NULL before actually setting pixbuf->storage.
-           */
-          case PROP_PIXELS:
-                  g_assert (pixbuf->s.pixels.pixels == NULL);
-                  notify = pixbuf->s.pixels.pixels != (guchar *) g_value_get_pointer (value);
-                  pixbuf->s.pixels.pixels = (guchar *) g_value_get_pointer (value);
-                  pixbuf->s.pixels.destroy_fn = NULL;
-                  pixbuf->s.pixels.destroy_fn_data = NULL;
-
-                  if (pixbuf->s.pixels.pixels != NULL) {
-                          pixbuf->storage = STORAGE_PIXELS;
-                  }
-                  break;
-          case PROP_PIXEL_BYTES:
-                  g_assert (pixbuf->s.bytes.bytes == NULL);
-                  notify = pixbuf->s.bytes.bytes != g_value_get_boxed (value);
-                  pixbuf->s.bytes.bytes = g_value_dup_boxed (value);
-
-                  if (pixbuf->s.bytes.bytes != NULL) {
-                          pixbuf->storage = STORAGE_BYTES;
-                  }
-                  break;
-          default:
-                  G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
-                  break;
-          }
+        GdkPixbuf *pixbuf = GDK_PIXBUF (object);
+        gboolean notify = TRUE;
+
+        switch (prop_id) {
+        case PROP_COLORSPACE:
+                notify = pixbuf->colorspace != g_value_get_enum (value);
+                pixbuf->colorspace = g_value_get_enum (value);
+                break;
+        case PROP_N_CHANNELS:
+                notify = pixbuf->n_channels != g_value_get_int (value);
+                pixbuf->n_channels = g_value_get_int (value);
+                break;
+        case PROP_HAS_ALPHA:
+                notify = pixbuf->has_alpha != g_value_get_boolean (value);
+                pixbuf->has_alpha = g_value_get_boolean (value);
+                break;
+        case PROP_BITS_PER_SAMPLE:
+                notify = pixbuf->bits_per_sample != g_value_get_int (value);
+                pixbuf->bits_per_sample = g_value_get_int (value);
+                break;
+        case PROP_WIDTH:
+                notify = pixbuf->width != g_value_get_int (value);
+                pixbuf->width = g_value_get_int (value);
+                break;
+        case PROP_HEIGHT:
+                notify = pixbuf->height != g_value_get_int (value);
+                pixbuf->height = g_value_get_int (value);
+                break;
+        case PROP_ROWSTRIDE:
+                notify = pixbuf->rowstride != g_value_get_int (value);
+                pixbuf->rowstride = g_value_get_int (value);
+                break;
+
+        /* The following two are a bit strange.  Both PROP_PIXELS and
+         * PROP_PIXEL_BYTES are G_PARAM_CONSTRUCT_ONLY properties, which means
+         * that GObject will generate default values for any missing one and
+         * call us for *both*.  So, we need to check whether the passed value is
+         * not NULL before actually setting pixbuf->storage.
+         */
+        case PROP_PIXELS: {
+                guchar *pixels = g_value_get_pointer (value);
+
+                if (pixels) {
+                        g_assert (pixbuf->storage == STORAGE_UNINITIALIZED);
+
+                        pixbuf->storage = STORAGE_PIXELS;
+                        pixbuf->s.pixels.pixels = pixels;
+                        pixbuf->s.pixels.destroy_fn = NULL;
+                        pixbuf->s.pixels.destroy_fn_data = NULL;
+                } else {
+                        notify = FALSE;
+                }
+
+                break;
+        }
+
+        case PROP_PIXEL_BYTES: {
+                GBytes *bytes = g_value_get_boxed (value);
+
+                if (bytes) {
+                        g_assert (pixbuf->storage == STORAGE_UNINITIALIZED);
+
+                        pixbuf->storage = STORAGE_BYTES;
+                        pixbuf->s.bytes.bytes = g_value_dup_boxed (value);
+                } else {
+                        notify = FALSE;
+                }
+
+                break;
+        }
+
+        default:
+                G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
+                break;
+        }
 
         if (notify)
                 g_object_notify_by_pspec (G_OBJECT (object), pspec);
@@ -1313,39 +1328,109 @@ gdk_pixbuf_get_property (GObject         *object,
 			 GValue          *value,
 			 GParamSpec      *pspec)
 {
-  GdkPixbuf *pixbuf = GDK_PIXBUF (object);
+        GdkPixbuf *pixbuf = GDK_PIXBUF (object);
   
-  switch (prop_id)
-          {
-          case PROP_COLORSPACE:
-                  g_value_set_enum (value, gdk_pixbuf_get_colorspace (pixbuf));
-                  break;
-          case PROP_N_CHANNELS:
-                  g_value_set_int (value, gdk_pixbuf_get_n_channels (pixbuf));
-                  break;
-          case PROP_HAS_ALPHA:
-                  g_value_set_boolean (value, gdk_pixbuf_get_has_alpha (pixbuf));
-                  break;
-          case PROP_BITS_PER_SAMPLE:
-                  g_value_set_int (value, gdk_pixbuf_get_bits_per_sample (pixbuf));
-                  break;
-          case PROP_WIDTH:
-                  g_value_set_int (value, gdk_pixbuf_get_width (pixbuf));
-                  break;
-          case PROP_HEIGHT:
-                  g_value_set_int (value, gdk_pixbuf_get_height (pixbuf));
-                  break;
-          case PROP_ROWSTRIDE:
-                  g_value_set_int (value, gdk_pixbuf_get_rowstride (pixbuf));
-                  break;
-          case PROP_PIXELS:
-                  g_value_set_pointer (value, gdk_pixbuf_get_pixels (pixbuf));
-                  break;
-          case PROP_PIXEL_BYTES:
-                  g_value_set_boxed (value, gdk_pixbuf_read_pixel_bytes (pixbuf));
-                  break;
-          default:
-                  G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
-                  break;
-          }
+        switch (prop_id) {
+        case PROP_COLORSPACE:
+                g_value_set_enum (value, gdk_pixbuf_get_colorspace (pixbuf));
+                break;
+        case PROP_N_CHANNELS:
+                g_value_set_int (value, gdk_pixbuf_get_n_channels (pixbuf));
+                break;
+        case PROP_HAS_ALPHA:
+                g_value_set_boolean (value, gdk_pixbuf_get_has_alpha (pixbuf));
+                break;
+        case PROP_BITS_PER_SAMPLE:
+                g_value_set_int (value, gdk_pixbuf_get_bits_per_sample (pixbuf));
+                break;
+        case PROP_WIDTH:
+                g_value_set_int (value, gdk_pixbuf_get_width (pixbuf));
+                break;
+        case PROP_HEIGHT:
+                g_value_set_int (value, gdk_pixbuf_get_height (pixbuf));
+                break;
+        case PROP_ROWSTRIDE:
+                g_value_set_int (value, gdk_pixbuf_get_rowstride (pixbuf));
+                break;
+        case PROP_PIXELS:
+                g_value_set_pointer (value, gdk_pixbuf_get_pixels (pixbuf));
+                break;
+        case PROP_PIXEL_BYTES:
+                g_value_set_boxed (value, gdk_pixbuf_read_pixel_bytes (pixbuf));
+                break;
+        default:
+                G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec);
+                break;
+        }
+}
+
+static void
+make_storage_invalid (GdkPixbuf *pixbuf)
+{
+        char *buf;
+        gsize bufsize = 3;
+
+        buf = g_new0(char, bufsize);
+
+        pixbuf->storage = STORAGE_BYTES;
+        pixbuf->s.bytes.bytes = g_bytes_new_with_free_func (buf, bufsize, g_free, NULL);
+
+        pixbuf->colorspace = GDK_COLORSPACE_RGB;
+        pixbuf->n_channels = 3;
+        pixbuf->bits_per_sample = 8;
+        pixbuf->width = 1;
+        pixbuf->height = 1;
+        pixbuf->rowstride = 3;
+        pixbuf->has_alpha = FALSE;
+}
+
+static void
+gdk_pixbuf_constructed (GObject *object)
+{
+        GdkPixbuf *pixbuf = GDK_PIXBUF (object);
+
+        G_OBJECT_CLASS (gdk_pixbuf_parent_class)->constructed (object);
+
+        switch (pixbuf->storage) {
+        case STORAGE_UNINITIALIZED:
+                /* This means that neither of the construct properties "pixels" nor "pixel-bytes"
+                 * was specified during a call to g_object_new().
+                 *
+                 * To avoid breaking ABI, we don't emit this warning.  We'll want
+                 * to emit it once we can have fallible construction.
+                 *
+                 * g_warning ("pixbuf needs to be constructed with the 'pixels' or 'pixel-bytes' properties");
+                 */
+
+                make_storage_invalid (pixbuf);
+                break;
+
+        case STORAGE_PIXELS:
+                g_assert (pixbuf->s.pixels.pixels != NULL);
+                break;
+
+        case STORAGE_BYTES: {
+                gsize bytes_size;
+                gint width, height;
+                gboolean has_alpha;
+
+                g_assert (pixbuf->s.bytes.bytes != NULL);
+
+                bytes_size = g_bytes_get_size (pixbuf->s.bytes.bytes);
+                width = pixbuf->width;
+                height = pixbuf->height;
+                has_alpha = pixbuf->has_alpha;
+
+                /* This is the same check as in gdk_pixbuf_new_from_bytes() */
+                if (!(bytes_size >= width * height * (has_alpha ? 4 : 3))) {
+                        g_error ("GBytes is too small to fit the pixbuf's declared width and height");
+                }
+                break;
+        }
+
+        default:
+                g_assert_not_reached ();
+        }
+
+        g_assert (pixbuf->storage != STORAGE_UNINITIALIZED);
 }
diff --git a/tests/meson.build b/tests/meson.build
index aaa1817e7..5116fc2b9 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -47,6 +47,7 @@ resources_h = custom_target('resources.h',
 #  - io: Loading/saving
 #  - ops: Pixel operations
 installed_tests = [
+  [ 'pixbuf-construction', ['conform'], ],
   [ 'animation', ['format'], ],
   [ 'cve-2015-4491', ['security'], true ],
   [ 'pixbuf-fail', ['conform', 'slow'], ],
diff --git a/tests/pixbuf-construction.c b/tests/pixbuf-construction.c
new file mode 100644
index 000000000..5e9e22b16
--- /dev/null
+++ b/tests/pixbuf-construction.c
@@ -0,0 +1,107 @@
+/* -*- Mode: C; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* GdkPixbuf library - tests for GdkPixbuf constructors
+ *
+ * Copyright (C) 2018 Federico Mena Quintero
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+#include "config.h"
+#include "gdk-pixbuf/gdk-pixbuf.h"
+#include "test-common.h"
+
+static void
+test_no_construct_properties (void)
+{
+  GdkPixbuf *pixbuf = g_object_new (GDK_TYPE_PIXBUF, NULL);
+  GBytes *bytes;
+  guchar *pixels;
+
+  g_assert_cmpint (gdk_pixbuf_get_width (pixbuf), ==, 1);
+  g_assert_cmpint (gdk_pixbuf_get_height (pixbuf), ==, 1);
+
+  g_object_get (pixbuf, "pixel-bytes", &bytes, NULL);
+  g_assert (bytes != NULL);
+  g_bytes_unref (bytes);
+
+  g_object_get (pixbuf, "pixels", &pixels, NULL);
+  g_assert (pixels != NULL);
+}
+
+#define WIDTH 10
+#define HEIGHT 20
+#define BUFFER_SIZE (WIDTH * HEIGHT * 4)
+#define ROWSTRIDE (WIDTH * 4)
+
+static void
+test_pixels (void)
+{
+  guchar *pixels = g_new0 (guchar, BUFFER_SIZE);
+
+  GdkPixbuf *pixbuf = g_object_new (GDK_TYPE_PIXBUF,
+				    "width", WIDTH,
+				    "height", HEIGHT,
+				    "rowstride", ROWSTRIDE,
+				    "bits-per-sample", 8,
+				    "n-channels", 3,
+				    "has-alpha", TRUE,
+				    "pixels", pixels,
+				    NULL);
+
+  g_assert (gdk_pixbuf_get_pixels (pixbuf) == pixels);
+  g_assert_cmpint (gdk_pixbuf_get_width (pixbuf), ==, WIDTH);
+  g_assert_cmpint (gdk_pixbuf_get_height (pixbuf), ==, HEIGHT);
+  g_assert_cmpint (gdk_pixbuf_get_rowstride (pixbuf), ==, ROWSTRIDE);
+
+  g_object_unref (pixbuf);
+  g_free (pixels);
+}
+
+static void
+test_bytes (void)
+{
+  guchar *pixels = g_new0 (guchar, BUFFER_SIZE);
+  GBytes *bytes = g_bytes_new_take (pixels, BUFFER_SIZE);
+  GdkPixbuf *pixbuf = g_object_new (GDK_TYPE_PIXBUF,
+				    "width", WIDTH,
+				    "height", HEIGHT,
+				    "rowstride", ROWSTRIDE,
+				    "bits-per-sample", 8,
+				    "n-channels", 3,
+				    "has-alpha", TRUE,
+				    "pixel-bytes", bytes,
+				    NULL);
+
+  GBytes *read_bytes = gdk_pixbuf_read_pixel_bytes (pixbuf);
+
+  g_assert (read_bytes == bytes);
+  g_assert_cmpint (gdk_pixbuf_get_width (pixbuf), ==, WIDTH);
+  g_assert_cmpint (gdk_pixbuf_get_height (pixbuf), ==, HEIGHT);
+  g_assert_cmpint (gdk_pixbuf_get_rowstride (pixbuf), ==, ROWSTRIDE);
+
+  g_bytes_unref (read_bytes);
+  g_object_unref (pixbuf);
+  g_bytes_unref (bytes);
+}
+
+int
+main (int argc, char **argv)
+{
+  g_test_init (&argc, &argv, NULL);
+
+  g_test_add_func ("/pixbuf/construction/no_construct_properties", test_no_construct_properties);
+  g_test_add_func ("/pixbuf/construction/pixels", test_pixels);
+  g_test_add_func ("/pixbuf/construction/bytes", test_bytes);
+
+  return g_test_run ();
+}