From 3c7740498fd31b6746dd7e04601886766a6644b7 Mon Sep 17 00:00:00 2001 From: Federico Mena Quintero Date: Wed, 12 Dec 2018 18:22:30 -0600 Subject: (#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 --- gdk-pixbuf/gdk-pixbuf-private.h | 1 + gdk-pixbuf/gdk-pixbuf.c | 279 ++++++++++++++++++++++++++-------------- tests/meson.build | 1 + tests/pixbuf-construction.c | 107 +++++++++++++++ 4 files changed, 291 insertions(+), 97 deletions(-) create mode 100644 tests/pixbuf-construction.c 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 . + */ +#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 (); +} -- cgit v1.2.1