Fix the response to a geometry that has no pixels

docs/docs/operations/transformations/crop.md

@@ -12,8 +12,12 @@ crop/100x100           100 by 100 from the centre
 crop/50%x50%+0+0       the top left quarter
 ```
 
-A region larger than the image is reduced to fit. An offset outside the image
-produces an empty result.
+A region larger than the image is reduced to fit, so a crop that overhangs an
+edge keeps the part that is on the image.
+
+A region that lies entirely off the image is refused with `400`. An offset of
+exactly the width or the height leaves nothing, so it is refused the same way.
+The offset one pixel inside the edge keeps a strip one pixel wide.
 
 Some clients escape a `+` as `%20`, which arrives as a space. `crop` converts a
 space back to a `+`, so `100x100%20350%200` works.

docs/docs/operations/transformations/resize.md

@@ -18,5 +18,8 @@ resize/50%          half the size
 
 A JPEG source is resampled at 2x1x1 chroma subsampling.
 
+A side never falls below one pixel. A percentage rounds down, so a small image
+scaled by a small percentage would otherwise ask for a side of no pixels.
+
 [`DimsOptimizeResize`](/configuration/output) makes ImageMagick decode a large
 JPEG at a smaller scale first, which is faster and slightly softer.

src/operations/crop.c

@@ -5,6 +5,7 @@
  */
 
 #include "operations.h"
+#include "geometry.h"
 
 apr_status_t
 dims_crop_operation (dims_request_rec *d, char *args, const char **err) {
@@ -41,7 +42,15 @@ dims_crop_operation (dims_request_rec *d, char *args, const char **err) {
 
     DestroyExceptionInfo(ex_info);
 
-    MAGICK_CHECK(MagickCropImage(d->wand, rec.width, rec.height, rec.x, rec.y), d);
+    /* A region that lies off the image has nothing to cut out. go-dims answers
+     * 400 for it, and a signature is portable between the two. */
+    if (!dims_geometry_crop_fits(GetImageFromMagickWand(d->wand), &rec)) {
+        *err = "The crop region lies outside the image";
+        return DIMS_BAD_ARGUMENTS;
+    }
+
+    MAGICK_CROP_CHECK(MagickCropImage(d->wand, rec.width, rec.height, rec.x, rec.y),
+                      d, err, "The crop region lies outside the image");
     MAGICK_CHECK(MagickSetImagePage(d->wand, rec.width, rec.height, rec.x, rec.y), d);
 
     return DIMS_SUCCESS;

src/operations/geometry.c

@@ -26,3 +26,49 @@ dims_parse_size_geometry(const Image *image, const char *geometry,
     return ParseMetaGeometry(geometry, &region->x, &region->y, &region->width,
                              &region->height);
 }
+
+int
+dims_geometry_crop_fits(const Image *image, RectangleInfo *region)
+{
+    ssize_t columns = (ssize_t) image->columns;
+    ssize_t rows = (ssize_t) image->rows;
+    ssize_t left = (region->x > 0) ? region->x : 0;
+    ssize_t top = (region->y > 0) ? region->y : 0;
+
+    /*
+     * A side of zero means the rest of the image. ParseGravityGeometry fills
+     * that in from the image, ParseAbsoluteGeometry does not, so the callers
+     * that use the second one arrive here with one.
+     */
+    if (region->width == 0 && columns > left) {
+        region->width = (size_t) (columns - left);
+    }
+    if (region->height == 0 && rows > top) {
+        region->height = (size_t) (rows - top);
+    }
+
+    if (region->width == 0 || region->height == 0) {
+        return 0;
+    }
+
+    /* Wholly right of, below, left of, or above the image. */
+    if (region->x >= columns || region->y >= rows ||
+            region->x + (ssize_t) region->width <= 0 ||
+            region->y + (ssize_t) region->height <= 0) {
+        return 0;
+    }
+
+    return 1;
+}
+
+void
+dims_geometry_least_one_pixel(RectangleInfo *region)
+{
+    if (region->width == 0) {
+        region->width = 1;
+    }
+
+    if (region->height == 0) {
+        region->height = 1;
+    }
+}

src/operations/geometry.h

@@ -20,4 +20,23 @@
 MagickStatusType dims_parse_size_geometry(const Image *image, const char *geometry,
                                           RectangleInfo *region);
 
+/*
+ * Completes a crop region and reports whether any of it lies on the image.
+ *
+ * A side of zero becomes the rest of the image, measured from the offset.
+ * Returns 0 when nothing remains, which happens when the offset sits at or
+ * past an edge. ImageMagick refuses that crop, and go-dims answers 400 for the
+ * same region, so the caller returns DIMS_BAD_ARGUMENTS and the two agree.
+ */
+int dims_geometry_crop_fits(const Image *image, RectangleInfo *region);
+
+/*
+ * Raises a width or a height of zero to one.
+ *
+ * A percentage rounds down, so a small enough image scaled by a small enough
+ * percentage asks for a side of no pixels. No format holds such an image, and
+ * ImageMagick refuses to build one.
+ */
+void dims_geometry_least_one_pixel(RectangleInfo *region);
+
 #endif

src/operations/legacy.c

@@ -39,11 +39,20 @@ dims_legacy_crop_operation (dims_request_rec *d, char *args, const char **err) {
     x = (width / 2) - (rec.width / 2);
     y = (height / 2) - (rec.height / 2);
 
+    rec.x = x;
+    rec.y = y;
+
+    if (!dims_geometry_crop_fits(GetImageFromMagickWand(d->wand), &rec)) {
+        *err = "The crop region lies outside the image";
+        return DIMS_BAD_ARGUMENTS;
+    }
+
     ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, d->r, 
         "legacy_crop will crop to %ldx%ld+%d+%d", 
         rec.width, rec.height, x, y);
 
-    MAGICK_CHECK(MagickCropImage(d->wand, rec.width, rec.height, x, y), d);
+    MAGICK_CROP_CHECK(MagickCropImage(d->wand, rec.width, rec.height, rec.x, rec.y),
+                      d, err, "The crop region lies outside the image");
 
     return DIMS_SUCCESS;
 }
@@ -62,6 +71,8 @@ dims_legacy_thumbnail_operation (dims_request_rec *d, char *args, const char **e
         return DIMS_FAILURE;
     }
 
+    dims_geometry_least_one_pixel(&rec);
+
     char *format = MagickGetImageFormat(d->wand);
     if (strcmp(format, "JPEG") == 0) {
         const double factors[3] = { 2.0, 1.0, 1.0 };
@@ -89,10 +100,19 @@ dims_legacy_thumbnail_operation (dims_request_rec *d, char *args, const char **e
     x = (width / 2) - (rec.width / 2);
     y = (height / 2) - (rec.height / 2);
 
+    rec.x = x;
+    rec.y = y;
+
+    if (!dims_geometry_crop_fits(GetImageFromMagickWand(d->wand), &rec)) {
+        *err = "The crop region lies outside the image";
+        return DIMS_BAD_ARGUMENTS;
+    }
+
     ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, d->r, 
         "legacy_thumbnail will crop to %ldx%ld+%d+%d", rec.width, rec.height, x, y);
 
-    MAGICK_CHECK(MagickCropImage(d->wand, rec.width, rec.height, x, y), d);
+    MAGICK_CROP_CHECK(MagickCropImage(d->wand, rec.width, rec.height, rec.x, rec.y),
+                      d, err, "The crop region lies outside the image");
 
     return DIMS_SUCCESS;
 }

src/operations/operations.h

@@ -34,4 +34,22 @@
         }                                           \
     } while (0)
 
+/*
+ * Runs a crop and returns a bad request when ImageMagick refuses it.
+ *
+ * The geometry checks above catch the regions go-dims names, and this catches
+ * the rest. A region ImageMagick will not cut is the caller's geometry, not a
+ * fault in the service, so it must not end the request with 500.
+ */
+#define MAGICK_CROP_CHECK(func, rec, err, message)  \
+    do {                                            \
+        apr_status_t code = func;                   \
+        if (rec->status == DIMS_IMAGEMAGICK_TIMEOUT) { \
+            return DIMS_IMAGEMAGICK_TIMEOUT;        \
+        } else if (code == MagickFalse) {           \
+            *(err) = (message);                     \
+            return DIMS_BAD_ARGUMENTS;              \
+        }                                           \
+    } while (0)
+
 #endif

src/operations/resize.c

@@ -18,6 +18,8 @@ dims_resize_operation (dims_request_rec *d, char *args, const char **err) {
         return DIMS_FAILURE;
     }
 
+    dims_geometry_least_one_pixel(&rec);
+
     char *format = MagickGetImageFormat(d->wand);
     if (strcmp(format, "JPEG") == 0) {
         const double factors[3] = { 2.0, 1.0, 1.0 };
@@ -32,6 +34,7 @@ dims_resize_operation (dims_request_rec *d, char *args, const char **err) {
         RectangleInfo sampleRec = rec;
         sampleRec.width *= d->optimize_resize;
         sampleRec.height *= d->optimize_resize;
+        dims_geometry_least_one_pixel(&sampleRec);
 
         orig_width = MagickGetImageWidth(d->wand);
         orig_height = MagickGetImageHeight(d->wand);

src/operations/thumbnail.c

@@ -19,6 +19,8 @@ dims_thumbnail_operation (dims_request_rec *d, char *args, const char **err) {
         return DIMS_FAILURE;
     }
 
+    dims_geometry_least_one_pixel(&rec);
+
     char *format = MagickGetImageFormat(d->wand);
     if (strcmp(format, "JPEG") == 0) {
         const double factors[3] = { 2.0, 1.0, 1.0 };
@@ -33,6 +35,7 @@ dims_thumbnail_operation (dims_request_rec *d, char *args, const char **err) {
         RectangleInfo sampleRec = rec;
         sampleRec.width *= d->optimize_resize;
         sampleRec.height *= d->optimize_resize;
+        dims_geometry_least_one_pixel(&sampleRec);
 
         orig_width = MagickGetImageWidth(d->wand);
         orig_height = MagickGetImageHeight(d->wand);
@@ -52,7 +55,13 @@ dims_thumbnail_operation (dims_request_rec *d, char *args, const char **err) {
             return DIMS_FAILURE;
         }
 
-        MAGICK_CHECK(MagickCropImage(d->wand, rec.width, rec.height, rec.x, rec.y), d);
+        if (!dims_geometry_crop_fits(GetImageFromMagickWand(d->wand), &rec)) {
+            *err = "The crop region lies outside the image";
+            return DIMS_BAD_ARGUMENTS;
+        }
+
+        MAGICK_CROP_CHECK(MagickCropImage(d->wand, rec.width, rec.height, rec.x, rec.y),
+                      d, err, "The crop region lies outside the image");
     }
 
     MAGICK_CHECK(MagickSetImagePage(d->wand, rec.width, rec.height, rec.x, rec.y), d);

src/operations/watermark.c

@@ -306,6 +306,15 @@ dims_watermark_operation (dims_request_rec *d, char *args, const char **err) {
         final_height = largest_size;
     }
 
+    /* A small source scales the overlay to less than a pixel, and no image has
+     * a side of none. */
+    if (final_width < 1) {
+        final_width = 1;
+    }
+    if (final_height < 1) {
+        final_height = 1;
+    }
+
     if (MagickScaleImage(overlay_wand, final_width, final_height) == MagickFalse) {
         status = (d->status == DIMS_IMAGEMAGICK_TIMEOUT)
                 ? DIMS_IMAGEMAGICK_TIMEOUT : DIMS_FAILURE;

test/http/test_crop.c

@@ -35,18 +35,53 @@ test_crop_region_larger_than_image(void)
                     -1, -1);
 }
 
+/*
+ * A region entirely outside the image has nothing to cut out, so the command
+ * is refused and this server answers with its error image. The status the
+ * refusal carries is on the server that has no error image, in
+ * test_no_error_image.c.
+ */
+static void
+assert_refused(const char *commands, const char *what)
+{
+    dims_response *fallback = dims_request_ops(commands, "missing.png");
+    dims_response *refused = dims_request_ops(commands, "grid.png");
+
+    CHECK(fallback->body_len == refused->body_len &&
+              memcmp(fallback->body, refused->body, refused->body_len) == 0,
+          "%s must be refused", what);
+
+    dims_response_free(fallback);
+    dims_response_free(refused);
+}
+
 static void
 test_crop_x_offset_outside_image(void)
 {
-    dims_run_golden("TestCropXOffsetOutsideImage", "grid.png", "crop/256x256+768+768",
-                    -1, -1);
+    assert_refused("crop/256x256+768+768", "an x offset past the image");
 }
 
 static void
 test_crop_y_offset_outside_image(void)
 {
-    dims_run_golden("TestCropYOffsetOutsideImage", "grid.png", "crop/256x256+0+768",
-                    -1, -1);
+    assert_refused("crop/256x256+0+768", "a y offset past the image");
+}
+
+/*
+ * An offset of exactly the width leaves a region no pixels wide. The offsets
+ * on either side of it are accepted, so only this one value used to end the
+ * request with 500.
+ *
+ * Not a port. go-dims covers the offsets further out, not the one on the edge.
+ */
+static void
+test_crop_offset_on_the_edge(void)
+{
+    assert_refused("crop/50x50+512+0", "an x offset of exactly the width");
+
+    /* One column remains, so the crop keeps a strip one pixel wide. */
+    dims_run_golden("TestCropOffsetOnTheEdge", "grid.png", "crop/50x50+511+0",
+                    1, 50);
 }
 
 const dims_test dims_tests_crop[] = {
@@ -56,5 +91,6 @@ const dims_test dims_tests_crop[] = {
     { "TestCropRegionLargerThanImage", test_crop_region_larger_than_image, NULL },
     { "TestCropXOffsetOutsideImage", test_crop_x_offset_outside_image, NULL },
     { "TestCropYOffsetOutsideImage", test_crop_y_offset_outside_image, NULL },
+    { "TestCropOffsetOnTheEdge", test_crop_offset_on_the_edge, NULL },
     DIMS_TEST_END
 };

test/http/test_no_error_image.c

@@ -186,6 +186,38 @@ test_watermark_short_argument_list_is_a_bad_request(void)
     free(url);
 }
 
+/*
+ * A crop region entirely off the image, and one whose offset is exactly the
+ * width. Both leave nothing to cut out. go-dims refuses the first pair, and
+ * the second used to answer 500 while the offsets on either side of it were
+ * accepted.
+ */
+static void
+test_crop_outside_the_image_is_a_bad_request(void)
+{
+    char *url = dims_fixture_url("grid.png");
+    char *path;
+
+    path = dims_sign_dims4("crop/256x256+768+768", url, NULL, NULL);
+    CHECK_INT(status_of(path), 400, "an x offset past the image");
+    free(path);
+
+    path = dims_sign_dims4("crop/256x256+0+768", url, NULL, NULL);
+    CHECK_INT(status_of(path), 400, "a y offset past the image");
+    free(path);
+
+    path = dims_sign_dims4("crop/50x50+512+0", url, NULL, NULL);
+    CHECK_INT(status_of(path), 400, "an x offset of exactly the width");
+    free(path);
+
+    /* One column remains, so this one is a crop rather than a refusal. */
+    path = dims_sign_dims4("crop/50x50+511+0", url, NULL, NULL);
+    CHECK_INT(status_of(path), 200, "one pixel inside the edge");
+    free(path);
+
+    free(url);
+}
+
 const dims_test dims_tests_no_error_image[] = {
     { "TestUnknownClientIsAServerError", test_unknown_client_is_a_server_error, NULL },
     { "TestWrongSignatureIsABadRequest", test_wrong_signature_is_a_bad_request, NULL },
@@ -196,6 +228,8 @@ const dims_test dims_tests_no_error_image[] = {
       test_quality_out_of_range_is_a_bad_request, NULL },
     { "TestWatermarkShortArgumentListIsABadRequest",
       test_watermark_short_argument_list_is_a_bad_request, NULL },
+    { "TestCropOutsideTheImageIsABadRequest",
+      test_crop_outside_the_image_is_a_bad_request, NULL },
     { "TestBadGeometryReportsTheModuleStatus",
       test_bad_geometry_reports_the_module_status, NULL },
     DIMS_TEST_END

test/http/test_resize.c

@@ -63,6 +63,16 @@ test_resize_fill_with_crop(void)
                     50, 100);
 }
 
+/*
+ * A percentage that rounds a side to no pixels answers with a single pixel
+ * rather than ending the request.
+ */
+static void
+test_resize_below_one_pixel(void)
+{
+    dims_run_golden("TestResizeBelowOnePixel", "grid.png", "resize/0.1%", 1, 1);
+}
+
 const dims_test dims_tests_resize[] = {
     { "TestResize", test_resize, NULL },
     { "TestResizeOnlySmaller", test_resize_only_smaller, NULL },
@@ -72,5 +82,6 @@ const dims_test dims_tests_resize[] = {
     { "TestResizeIgnoreAspectRatioWidthOnly", test_resize_ignore_aspect_ratio_width_only, NULL },
     { "TestResizeFill", test_resize_fill, NULL },
     { "TestResizeFillWithCrop", test_resize_fill_with_crop, NULL },
+    { "TestResizeBelowOnePixel", test_resize_below_one_pixel, NULL },
     DIMS_TEST_END
 };