Unsigned overflow

Remove all currently detected cases of unsigned overflow.  Detection is
runtime, so test case dependent.  The changes to pngvalid.c eliminate
spurious and probably invalid tests with one while loop exception.

Apart from that and the change to the dependence on the intended
unsigned overflow in pngtrans.c the changes are limited to altering the
meme for an unsigned 'x' from:

   while (x-- > 0)

to

   for (; x > 0; --x)

This works because, in all cases, the control variable is not used in
the loop.  The 'while' meme was, at one time, warn'ed by GCC so it is
probably a good change, for some weird religious value of good.

Signed-off-by: John Bowler <jbowler@acm.org>

contrib/libtests/pngunknown.c

@@ -478,7 +478,7 @@ get_valid(display *d, png_infop info_ptr)
       png_textp text;
       png_uint_32 ntext = png_get_text(d->png_ptr, info_ptr, &text, NULL);
 
-      while (ntext-- > 0) switch (text[ntext].compression)
+      while (ntext > 0) switch (text[--ntext].compression)
       {
          case -1:
             flags |= PNG_INFO_tEXt;

contrib/libtests/pngvalid.c

@@ -1242,7 +1242,7 @@ store_image_check(const png_store* ps, png_const_structp pp, int iImage)
 
       image += 2; /* skip image first row markers */
 
-      while (rows-- > 0)
+      for (; rows > 0; --rows)
       {
          if (image[-2] != 190 || image[-1] != 239)
             png_error(pp, "row start overwritten");
@@ -11427,23 +11427,36 @@ perform_interlace_macro_validation(void)
        */
       for (v=0;;)
       {
+         /* The first two tests overflow if the pass row or column is outside
+          * the possible range for a 32-bit result.  In fact the values should
+          * never be outside the range for a 31-bit result, but checking for 32
+          * bits here ensures that if an app uses a bogus pass row or column
+          * (just so long as it fits in a 32 bit integer) it won't get a
+          * possibly dangerous overflow.
+          */
          /* First the base 0 stuff: */
-         m = PNG_ROW_FROM_PASS_ROW(v, pass);
+         if (v < png_pass_rows(0xFFFFFFFFU, pass))
-         f = png_row_from_pass_row(v, pass);
-         if (m != f)
          {
-            fprintf(stderr, "PNG_ROW_FROM_PASS_ROW(%u, %d) = %u != %x\n",
+            m = PNG_ROW_FROM_PASS_ROW(v, pass);
-               v, pass, m, f);
+            f = png_row_from_pass_row(v, pass);
-            exit(99);
+            if (m != f)
+            {
+               fprintf(stderr, "PNG_ROW_FROM_PASS_ROW(%u, %d) = %u != %x\n",
+                  v, pass, m, f);
+               exit(99);
+            }
          }
 
-         m = PNG_COL_FROM_PASS_COL(v, pass);
+         if (v < png_pass_cols(0xFFFFFFFFU, pass))
-         f = png_col_from_pass_col(v, pass);
-         if (m != f)
          {
-            fprintf(stderr, "PNG_COL_FROM_PASS_COL(%u, %d) = %u != %x\n",
+            m = PNG_COL_FROM_PASS_COL(v, pass);
-               v, pass, m, f);
+            f = png_col_from_pass_col(v, pass);
-            exit(99);
+            if (m != f)
+            {
+               fprintf(stderr, "PNG_COL_FROM_PASS_COL(%u, %d) = %u != %x\n",
+                  v, pass, m, f);
+               exit(99);
+            }
          }
 
          m = PNG_ROW_IN_INTERLACE_PASS(v, pass);

pngpriv.h

@@ -669,6 +669,24 @@
     ((png_size_t)(width) * (((png_size_t)(pixel_bits)) >> 3)) : \
     (( ((png_size_t)(width) * ((png_size_t)(pixel_bits))) + 7) >> 3) )
 
+/* This returns the number of trailing bits in the last byte of a row, 0 if the
+ * last byte is completely full of pixels.  It is, in principle, (pixel_bits x
+ * width) % 8, but that would overflow for large 'width'.  The second macro is
+ * the same except that it returns the number of unused bits in the last byte;
+ * (8-TRAILBITS), but 0 when TRAILBITS is 0.
+ *
+ * NOTE: these macros are intended to be self-evidently correct and never
+ * overflow on the assumption that pixel_bits is in the range 0..255.  The
+ * arguments are evaluated only once and they can be signed (e.g. as a result of
+ * the integral promotions).  The result of the expression always has type
+ * (png_uint_32), however the compiler always knows it is in the range 0..7.
+ */
+#define PNG_TRAILBITS(pixel_bits, width) \
+    (((pixel_bits) * ((width) % (png_uint_32)8)) % 8)
+
+#define PNG_PADBITS(pixel_bits, width) \
+    ((8 - PNG_TRAILBITS(pixel_bits, width)) % 8)
+
 /* PNG_OUT_OF_RANGE returns true if value is outside the range
  * ideal-delta..ideal+delta.  Each argument is evaluated twice.
  * "ideal" and "delta" should be constants, normally simple

pngread.c

@@ -3231,7 +3231,7 @@ png_image_read_colormapped(png_voidp argument)
          png_uint_32      y = image->height;
          png_bytep        row = png_voidcast(png_bytep, display->first_row);
 
-         while (y-- > 0)
+         for (; y > 0; --y)
          {
             png_read_row(png_ptr, row, NULL);
             row += row_bytes;
@@ -4064,7 +4064,7 @@ png_image_read_direct(png_voidp argument)
          png_uint_32      y = image->height;
          png_bytep        row = png_voidcast(png_bytep, display->first_row);
 
-         while (y-- > 0)
+         for (; y > 0; --y)
          {
             png_read_row(png_ptr, row, NULL);
             row += row_bytes;

pngtrans.c

@@ -693,7 +693,7 @@ png_do_check_palette_indexes(png_structrp png_ptr, png_row_infop row_info)
        * and this calculation is used because it avoids warnings that other
        * forms produced on either GCC or MSVC.
        */
-      int padding = (-row_info->pixel_depth * row_info->width) & 7;
+      int padding = PNG_PADBITS(row_info->pixel_depth, row_info->width);
       png_bytep rp = png_ptr->row_buf + row_info->rowbytes;
 
       switch (row_info->bit_depth)

pngwrite.c

@@ -1554,7 +1554,7 @@ png_write_image_16bit(png_voidp argument)
     */
    row_end = output_row + image->width * (channels+1);
 
-   while (y-- > 0)
+   for (; y > 0; --y)
    {
       png_const_uint_16p in_ptr = input_row;
       png_uint_16p out_ptr = output_row;
@@ -1705,7 +1705,7 @@ png_write_image_8bit(png_voidp argument)
       /* Use row_end in place of a loop counter: */
       row_end = output_row + image->width * (channels+1);
 
-      while (y-- > 0)
+      for (; y > 0; --y)
       {
          png_const_uint_16p in_ptr = input_row;
          png_bytep out_ptr = output_row;
@@ -1746,7 +1746,7 @@ png_write_image_8bit(png_voidp argument)
        */
       png_bytep row_end = output_row + image->width * channels;
 
-      while (y-- > 0)
+      for (; y > 0; --y)
       {
          png_const_uint_16p in_ptr = input_row;
          png_bytep out_ptr = output_row;
@@ -2136,7 +2136,7 @@ png_image_write_main(png_voidp argument)
       ptrdiff_t row_bytes = display->row_bytes;
       png_uint_32 y = image->height;
 
-      while (y-- > 0)
+      for (; y > 0; --y)
       {
          png_write_row(png_ptr, row);
          row += row_bytes;