png: Clean up PNG savingstatic-analysis

Iterate over the key/value arrays once, using GArray to store the text
chunks after validating the keys.

1 files changed, 263 insertions, 271 deletions

diff --git a/gdk-pixbuf/io-png.c b/gdk-pixbuf/io-png.c
index 3778f3469..26130fddb 100644
--- a/gdk-pixbuf/io-png.c
+++ b/gdk-pixbuf/io-png.c
@@ -885,300 +885,288 @@ png_save_to_callback_flush_func (png_structp png_ptr)
         ;
 }
 
-static gboolean real_save_png (GdkPixbuf        *pixbuf, 
-                               gchar           **keys,
-                               gchar           **values,
-                               GError          **error,
-                               gboolean          to_callback,
-                               FILE             *f,
-                               GdkPixbufSaveFunc save_func,
-                               gpointer          user_data)
+static gboolean
+real_save_png (GdkPixbuf        *pixbuf,
+               int               n_keys,
+               gchar           **keys,
+               gchar           **values,
+               GError          **error,
+               gboolean          to_callback,
+               FILE             *f,
+               GdkPixbufSaveFunc save_func,
+               gpointer          user_data)
 {
-       png_structp png_ptr = NULL;
-       png_infop info_ptr;
-       png_textp text_ptr = NULL;
-       guchar *ptr;
-       guchar *pixels;
-       int y;
-       int i;
-       png_bytep row_ptr;
-       png_color_8 sig_bit;
-       int w, h, rowstride;
-       int has_alpha;
-       int bpc;
-       int num_keys;
-       int compression = -1;
-       int x_density = 0;
-       int y_density = 0;
-       gboolean success = TRUE;
-       guchar *icc_profile = NULL;
-       gsize icc_profile_size = 0;
-       SaveToFunctionIoPtr to_callback_ioptr;
-
-       num_keys = 0;
-
-       if (keys && *keys) {
-               gchar **kiter = keys;
-               gchar **viter = values;
-
-               while (*kiter) {
-                       if (strncmp (*kiter, "tEXt::", 6) == 0) {
-                               gchar  *key = *kiter + 6;
-                               int     len = strlen (key);
-                               if (len < 1 || len > 79) {
-                                       g_set_error_literal (error,
-                                                            GDK_PIXBUF_ERROR,
-                                                            GDK_PIXBUF_ERROR_BAD_OPTION,
-                                                            _("Keys for PNG text chunks must have at least 1 and at most 79 characters."));
-                                       success = FALSE;
-                                       goto cleanup;
-                               }
-                               for (i = 0; i < len; i++) {
-                                       if ((guchar) key[i] > 127) {
-                                               g_set_error_literal (error,
-                                                                    GDK_PIXBUF_ERROR,
-                                                                    GDK_PIXBUF_ERROR_BAD_OPTION,
-                                                                    _("Keys for PNG text chunks must be ASCII characters."));
-                                               success = FALSE;
-                                               goto cleanup;
-                                       }
-                               }
-                               num_keys++;
-                       } else if (strcmp (*kiter, "icc-profile") == 0) {
-                               /* decode from base64 */
-                               icc_profile = g_base64_decode (*viter, &icc_profile_size);
-                               if (icc_profile_size < 127) {
-                                       /* This is a user-visible error */
-                                       g_set_error (error,
-                                                    GDK_PIXBUF_ERROR,
-                                                    GDK_PIXBUF_ERROR_BAD_OPTION,
-                                                    _("Color profile has invalid length %d."),
-                                                    (gint)icc_profile_size);
-                                       success = FALSE;
-                                       goto cleanup;
-                               }
-                       } else if (strcmp (*kiter, "compression") == 0) {
-                               char *endptr = NULL;
-                               compression = strtol (*viter, &endptr, 10);
-
-                               if (endptr == *viter) {
-                                       g_set_error (error,
-                                                    GDK_PIXBUF_ERROR,
-                                                    GDK_PIXBUF_ERROR_BAD_OPTION,
-                                                    _("PNG compression level must be a value between 0 and 9; value “%s” could not be parsed."),
-                                                    *viter);
-                                       success = FALSE;
-                                       goto cleanup;
-                               }
-                               if (compression < 0 || compression > 9) {
-                                       /* This is a user-visible error;
-                                        * lets people skip the range-checking
-                                        * in their app.
-                                        */
-                                       g_set_error (error,
-                                                    GDK_PIXBUF_ERROR,
-                                                    GDK_PIXBUF_ERROR_BAD_OPTION,
-                                                    _("PNG compression level must be a value between 0 and 9; value “%d” is not allowed."),
-                                                    compression);
-                                       success = FALSE;
-                                       goto cleanup;
-                               }
-                       } else if (strcmp (*kiter, "x-dpi") == 0) {
-                               char *endptr = NULL;
-                               x_density = strtol (*viter, &endptr, 10);
-                               if (endptr == *viter)
-                                       x_density = -1;
-
-                               if (x_density <= 0) {
-                                       /* This is a user-visible error;
-                                        * lets people skip the range-checking
-                                        * in their app.
-                                        */
-                                       g_set_error (error,
-                                                    GDK_PIXBUF_ERROR,
-                                                    GDK_PIXBUF_ERROR_BAD_OPTION,
-                                                    _("PNG x-dpi must be greater than zero; value “%s” is not allowed."),
-                                                    *viter);
-
-                                       success = FALSE;
-                                       goto cleanup;
-                               }
-                       } else if (strcmp (*kiter, "y-dpi") == 0) {
-                               char *endptr = NULL;
-                               y_density = strtol (*viter, &endptr, 10);
-                               if (endptr == *viter)
-                                       y_density = -1;
-
-                               if (y_density <= 0) {
-                                       /* This is a user-visible error;
-                                        * lets people skip the range-checking
-                                        * in their app.
-                                        */
-                                       g_set_error (error,
-                                                    GDK_PIXBUF_ERROR,
-                                                    GDK_PIXBUF_ERROR_BAD_OPTION,
-                                                    _("PNG y-dpi must be greater than zero; value “%s” is not allowed."),
-                                                    *viter);
-
-                                       success = FALSE;
-                                       goto cleanup;
-                               }
-                       } else {
-                               g_warning ("Unrecognized parameter (%s) passed to PNG saver.", *kiter);
-                       }
-
-                       ++kiter;
-                       ++viter;
-               }
-       }
-
-       if (num_keys > 0) {
-               gchar **kiter = keys;
-               gchar **viter = values;
-
-               text_ptr = g_new0 (png_text, num_keys);
-               for (i = 0; i < num_keys; i++) {
-                       if (strncmp (*kiter, "tEXt::", 6) != 0) {
-                                kiter++;
-                                viter++;
-                       }
-
-                       text_ptr[i].compression = PNG_TEXT_COMPRESSION_NONE;
-                       text_ptr[i].key  = *kiter + 6;
-                       text_ptr[i].text = g_convert (*viter, -1, 
-                                                     "ISO-8859-1", "UTF-8", 
-                                                     NULL, &text_ptr[i].text_length, 
-                                                     NULL);
+        png_structp png_ptr = NULL;
+        png_infop info_ptr;
+        guchar *ptr;
+        guchar *pixels;
+        int y;
+        png_bytep row_ptr;
+        png_color_8 sig_bit;
+        int w, h, rowstride;
+        int has_alpha;
+        int bpc;
+        int compression = -1;
+        int x_density = 0;
+        int y_density = 0;
+        gboolean success = TRUE;
+        guchar *icc_profile = NULL;
+        gsize icc_profile_size = 0;
+        SaveToFunctionIoPtr to_callback_ioptr;
+        int num_keys = 0;
+        png_textp text_ptr = NULL;
+        GArray *text_data = NULL;
+
+        text_data = g_array_sized_new (FALSE, TRUE, sizeof (png_text), n_keys);
+
+        for (int i = 0; i < n_keys; i++) {
+                const char *key = keys[i];
+                const char *value = values[i];
+
+                if (strncmp (key, "tEXt::", 6) == 0) {
+                        const char *unprefixed_key = key + 6;
+                        int len = strlen (unprefixed_key);
+                        png_text text;
+
+                        if (len < 1 || len > 79) {
+                                /* Translators notice: '%s' is the name of the
+                                 * PNG text key
+                                 */
+                                g_set_error (error, GDK_PIXBUF_ERROR,
+                                             GDK_PIXBUF_ERROR_BAD_OPTION,
+                                             _("Invalid key “%s”. Keys for PNG text chunks must have at least 1 and at most 79 characters."),
+                                             unprefixed_key);
+                                success = FALSE;
+                                goto cleanup;
+                        }
+
+                        for (int i = 0; i < len; i++) {
+                                if ((guchar) unprefixed_key[i] > 127) {
+                                        /* Translators notice: '%s' is the name of
+                                         * the PNG text key
+                                         */
+                                        g_set_error (error, GDK_PIXBUF_ERROR,
+                                                     GDK_PIXBUF_ERROR_BAD_OPTION,
+                                                     _("Invalid key “%s”. Keys for PNG text chunks must be ASCII characters."),
+                                                     unprefixed_key);
+                                        success = FALSE;
+                                        goto cleanup;
+                                }
+                        }
+
+                        text.compression = PNG_TEXT_COMPRESSION_NONE;
+                        text.key = unprefixed_key;
+                        text.text = g_convert (value, -1,
+                                               "ISO-8859-1", "UTF-8",
+                                               NULL,
+                                               &text.text_length,
+                                               NULL);
 
 #ifdef PNG_iTXt_SUPPORTED 
-                       if (!text_ptr[i].text) {
-                               text_ptr[i].compression = PNG_ITXT_COMPRESSION_NONE;
-                               text_ptr[i].text = g_strdup (*viter);
-                               text_ptr[i].text_length = 0;
-                               text_ptr[i].itxt_length = strlen (text_ptr[i].text);
-                               text_ptr[i].lang = NULL;
-                               text_ptr[i].lang_key = NULL;
-                       }
+                        if (text.text == NULL) {
+                                text.compression = PNG_ITXT_COMPRESSION_NONE;
+                                text.text = g_strdup (value);
+                                text.text_length = 0;
+                                text.itxt_length = strlen (value);
+                                text.lang = NULL;
+                                text.lang_key = NULL;
+                        }
 #endif
 
-                       if (!text_ptr[i].text) {
-                               gint j;
-                               g_set_error (error,
+                        if (text.text == NULL) {
+                                g_set_error (error,
                                             GDK_PIXBUF_ERROR,
                                             GDK_PIXBUF_ERROR_BAD_OPTION,
-                                            _("Value for PNG text chunk %s cannot be converted to ISO-8859-1 encoding."), *kiter + 6);
-                               for (j = 0; j < i; j++)
-                                       g_free (text_ptr[j].text);
-                               g_free (text_ptr);
-                               return FALSE;
-                       }
-
-                        kiter++;
-                        viter++;
-               }
-       }
-
-       bpc = gdk_pixbuf_get_bits_per_sample (pixbuf);
-       w = gdk_pixbuf_get_width (pixbuf);
-       h = gdk_pixbuf_get_height (pixbuf);
-       rowstride = gdk_pixbuf_get_rowstride (pixbuf);
-       has_alpha = gdk_pixbuf_get_has_alpha (pixbuf);
-       pixels = gdk_pixbuf_get_pixels (pixbuf);
-
-       /* Guaranteed by the caller. */
-       g_assert (w >= 0);
-       g_assert (h >= 0);
-       g_assert (rowstride >= 0);
-
-       png_ptr = png_create_write_struct (PNG_LIBPNG_VER_STRING,
-                                          error,
-                                          png_simple_error_callback,
-                                          png_simple_warning_callback);
-       if (png_ptr == NULL) {
-	       success = FALSE;
-	       goto cleanup;
-       }
-
-       info_ptr = png_create_info_struct (png_ptr);
-       if (info_ptr == NULL) {
-	       success = FALSE;
-	       goto cleanup;
-       }
-       if (setjmp (png_jmpbuf(png_ptr))) {
-	       success = FALSE;
-	       goto cleanup;
-       }
-
-       if (num_keys > 0) {
-               png_set_text (png_ptr, info_ptr, text_ptr, num_keys);
-       }
-
-       if (to_callback) {
-               to_callback_ioptr.save_func = save_func;
-               to_callback_ioptr.user_data = user_data;
-               to_callback_ioptr.error = error;
-               png_set_write_fn (png_ptr, &to_callback_ioptr,
-                                 png_save_to_callback_write_func,
-                                 png_save_to_callback_flush_func);
-       } else {
-               png_init_io (png_ptr, f);
-       }
-
-       if (compression >= 0)
-               png_set_compression_level (png_ptr, compression);
+                                            _("Value for PNG text chunk '%s' cannot be converted to ISO-8859-1 encoding."), unprefixed_key);
+                                success = FALSE;
+                                goto cleanup;
+                        }
+
+                        g_array_append_val (text_data, text);
+                } else if (strcmp (key, "icc-profile") == 0) {
+                        icc_profile = g_base64_decode (value, &icc_profile_size);
+
+                        if (icc_profile_size < 127) {
+                                g_set_error (error, GDK_PIXBUF_ERROR,
+                                             GDK_PIXBUF_ERROR_BAD_OPTION,
+                                             _("Color profile has invalid length %d"),
+                                             (int) icc_profile_size);
+                                success = FALSE;
+                                goto cleanup;
+                        }
+                } else if (strcmp (key, "compression") == 0) {
+                        char *endptr = NULL;
+
+                        compression = strtol (value, &endptr, 10);
+                        if (endptr == value || (compression < 0 || compression > 9)) {
+                                g_set_error (error, GDK_PIXBUF_ERROR,
+                                             GDK_PIXBUF_ERROR_BAD_OPTION,
+                                             _("PNG compression level must be a value between 0 and 9; value “%s” is invalid"),
+                                             value);
+                                success = FALSE;
+                                goto cleanup;
+                        }
+                } else if (strcmp (key, "x-dpi") == 0 || strcmp (key, "y-dpi") == 0) {
+                        gboolean is_horizontal = strcmp (key, "x-dpi") == 0;
+                        char *endptr = NULL;
+
+                        int dpi = strtol (value, &endptr, 10);
+
+                        if (endptr == value || dpi <= 0) {
+                                g_set_error (error, GDK_PIXBUF_ERROR,
+                                             GDK_PIXBUF_ERROR_BAD_OPTION,
+                                             _("PNG %s must be greater than zero; value “%s” is not allowed"),
+                                             is_horizontal ? "x-dpi" : "y-dpi",
+                                             value);
+                                success = FALSE;
+                                goto cleanup;
+                        }
+
+                        if (is_horizontal) {
+                                x_density = dpi;
+                        } else {
+                                y_density = dpi;
+                        }
+                } else {
+                        g_warning ("Unrecognized parameter “%s” passed to the PNG saver", key);
+                }
+        }
+
+        bpc = gdk_pixbuf_get_bits_per_sample (pixbuf);
+        w = gdk_pixbuf_get_width (pixbuf);
+        h = gdk_pixbuf_get_height (pixbuf);
+        rowstride = gdk_pixbuf_get_rowstride (pixbuf);
+        has_alpha = gdk_pixbuf_get_has_alpha (pixbuf);
+        pixels = gdk_pixbuf_get_pixels (pixbuf);
+
+        if (text_data->len > 0) {
+                num_keys = text_data->len;
+                text_ptr = (png_textp) g_array_free (text_data, FALSE);
+                text_data = NULL;
+        } else {
+                g_clear_pointer (&text_data, g_array_unref);
+                num_keys = 0;
+                text_ptr = NULL;
+        }
+
+        /* Guaranteed by the caller. */
+        g_assert (w >= 0);
+        g_assert (h >= 0);
+        g_assert (rowstride >= 0);
+
+        png_ptr = png_create_write_struct (PNG_LIBPNG_VER_STRING,
+                                           error,
+                                           png_simple_error_callback,
+                                           png_simple_warning_callback);
+        if (png_ptr == NULL) {
+	        success = FALSE;
+	        goto cleanup;
+        }
+
+        info_ptr = png_create_info_struct (png_ptr);
+        if (info_ptr == NULL) {
+	        success = FALSE;
+	        goto cleanup;
+        }
+
+        if (setjmp (png_jmpbuf (png_ptr))) {
+	        success = FALSE;
+	        goto cleanup;
+        }
+
+        if (num_keys > 0) {
+                png_set_text (png_ptr, info_ptr, text_ptr, num_keys);
+        }
+
+        if (to_callback) {
+                to_callback_ioptr.save_func = save_func;
+                to_callback_ioptr.user_data = user_data;
+                to_callback_ioptr.error = error;
+                png_set_write_fn (png_ptr, &to_callback_ioptr,
+                                  png_save_to_callback_write_func,
+                                  png_save_to_callback_flush_func);
+        } else {
+                png_init_io (png_ptr, f);
+        }
+
+        if (compression >= 0) {
+                png_set_compression_level (png_ptr, compression);
+        }
 
 #ifdef PNG_pHYs_SUPPORTED
-       if (x_density > 0 && y_density > 0)
-               png_set_pHYs (png_ptr, info_ptr, DPI_TO_DPM (x_density), DPI_TO_DPM (y_density), PNG_RESOLUTION_METER);
+        if (x_density > 0 && y_density > 0) {
+                png_set_pHYs (png_ptr, info_ptr,
+                              DPI_TO_DPM (x_density),
+                              DPI_TO_DPM (y_density),
+                              PNG_RESOLUTION_METER);
+        }
 #endif
 
 #if defined(PNG_iCCP_SUPPORTED)
         /* the proper ICC profile title is encoded in the profile */
         if (icc_profile != NULL) {
                 png_set_iCCP (png_ptr, info_ptr,
-                              "ICC profile", PNG_COMPRESSION_TYPE_BASE,
-                              (png_bytep) icc_profile, icc_profile_size);
+                              "ICC profile",
+                              PNG_COMPRESSION_TYPE_BASE,
+                              (png_bytep) icc_profile,
+                              icc_profile_size);
         }
 #endif
 
-       if (has_alpha) {
-               png_set_IHDR (png_ptr, info_ptr, w, h, bpc,
-                             PNG_COLOR_TYPE_RGB_ALPHA, PNG_INTERLACE_NONE,
-                             PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);
-       } else {
-               png_set_IHDR (png_ptr, info_ptr, w, h, bpc,
-                             PNG_COLOR_TYPE_RGB, PNG_INTERLACE_NONE,
-                             PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);
-       }
-       /* Note bpc is always 8 */
-       sig_bit.red = bpc;
-       sig_bit.green = bpc;
-       sig_bit.blue = bpc;
-       sig_bit.alpha = bpc;
-       png_set_sBIT (png_ptr, info_ptr, &sig_bit);
-       png_write_info (png_ptr, info_ptr);
-       png_set_packing (png_ptr);
-
-       for (y = 0, ptr = pixels; y < h; y++, ptr += rowstride) {
-               row_ptr = (png_bytep)ptr;
-               png_write_rows (png_ptr, &row_ptr, 1);
-       }
-
-       png_write_end (png_ptr, info_ptr);
+        if (has_alpha) {
+                png_set_IHDR (png_ptr, info_ptr, w, h, bpc,
+                              PNG_COLOR_TYPE_RGB_ALPHA,
+                              PNG_INTERLACE_NONE,
+                              PNG_COMPRESSION_TYPE_BASE,
+                              PNG_FILTER_TYPE_BASE);
+        } else {
+                png_set_IHDR (png_ptr, info_ptr, w, h, bpc,
+                              PNG_COLOR_TYPE_RGB,
+                              PNG_INTERLACE_NONE,
+                              PNG_COMPRESSION_TYPE_BASE,
+                              PNG_FILTER_TYPE_BASE);
+        }
+
+        /* Note bpc is always 8 */
+        sig_bit.red = bpc;
+        sig_bit.green = bpc;
+        sig_bit.blue = bpc;
+        sig_bit.alpha = bpc;
+        png_set_sBIT (png_ptr, info_ptr, &sig_bit);
+        png_write_info (png_ptr, info_ptr);
+        png_set_packing (png_ptr);
+
+        for (y = 0, ptr = pixels; y < h; y++, ptr += rowstride) {
+                row_ptr = (png_bytep)ptr;
+                png_write_rows (png_ptr, &row_ptr, 1);
+        }
+
+        png_write_end (png_ptr, info_ptr);
+
+        for (int i = 0; i < num_keys; i++) {
+                g_free (text_ptr[i].text);
+        }
+
+        g_free (text_ptr);
 
 cleanup:
-        if (png_ptr != NULL)
+        if (png_ptr != NULL) {
                 png_destroy_write_struct (&png_ptr, &info_ptr);
+        }
 
-        g_free (icc_profile);
+        if (text_data != NULL) {
+                for (guint i = 0; i < text_data->len; i++) {
+                        png_textp text = &g_array_index (text_data, png_text, i);
 
-        if (text_ptr != NULL) {
-                for (i = 0; i < num_keys; i++)
-                        g_free (text_ptr[i].text);
-                g_free (text_ptr);
+                        g_free (text->text);
+                }
+
+                g_array_unref (text_data);
         }
 
-       return success;
+        g_free (icc_profile);
+
+        return success;
 }
 
 static gboolean
@@ -1188,7 +1176,9 @@ gdk_pixbuf__png_image_save (FILE          *f,
                             gchar        **values,
                             GError       **error)
 {
-        return real_save_png (pixbuf, keys, values, error,
+        int n_keys = keys != NULL ? g_strv_length (keys) : 0;
+
+        return real_save_png (pixbuf, n_keys, keys, values, error,
                               FALSE, f, NULL, NULL);
 }
 
@@ -1200,7 +1190,9 @@ gdk_pixbuf__png_image_save_to_callback (GdkPixbufSaveFunc   save_func,
                                         gchar             **values,
                                         GError            **error)
 {
-        return real_save_png (pixbuf, keys, values, error,
+        int n_keys = keys != NULL ? g_strv_length (keys) : 0;
+
+        return real_save_png (pixbuf, n_keys, keys, values, error,
                               TRUE, NULL, save_func, user_data);
 }