Cropping API doesn't check bounds, easy to misuse

[Shnatsel]

In image v0.25.2, the crop() and crop_imm() functions return an image view or an image unconditionally, even if the area to be cropped is completely out of bounds. This makes the API very easy to misuse:

use image; // 0.25.1
use image::{DynamicImage, GenericImageView};

pub fn main() {
    let image = DynamicImage::new_rgba8(50, 50);
    let cropped = image.crop_imm(100, 100, 100, 100); //  Succeeds despite wrong bounds!
    // and when we try to actually use it later...
    cropped.get_pixel(0,0); // PANIC!
}

Demo in playground: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=0898257582886870218d41c94506e3cb

[Shnatsel]

SubImages seem to be kinda broken in general: https://github.com/image-rs/image/issues/1412

[soumyasen1809]

@Shnatsel Hi, I am new to Rust and would like to take a shot at this issue. Is it possible to work on it?

I was thinking of a simple solution, like https://github.com/image-rs/image/blob/40940d2a330b0c923481fc04fa06a852692224c4/src/imageops/mod.rs#L49-L60 We can only return a SubImage if the width and height is > 0, else we return None

let (x, y, width, height) = crop_dimms(image, x, y, width, height);
if width > 0 && height > 0 { return Some(SubImage::new(image, x, y, width, height);) }
return None;

[Shnatsel]

This will require changing the public API, so it's not a change that will be easy to get merged. I suggest looking at issues that do not have the API tag for your first contribution.

[kornelski]

Since the result is useless anyway, I don't think it would be a breaking change to return a slightly less broken result?

Crop could try to stay within bounds of the image: clamp left & top coordinates to width-1 & height-1, and then clamp size to whatever is left.

[soumyasen1809]

If we assign the (x, y) to (width-1, height-1), and the size left is > 0, then we will always return a single pixel as a Subimage. Isn't that wrong? Because the result is a single pixel, while there should not be any pixels in the output Subimago (i.e. the Subimage should not exist/ be defined).

[kornelski]

It is wrong, but the wrongness started with invalid input. A 1x1 image may be better than a crashy empty one. Alternatively, the function could itself panic on out-of-bounds coordinates. The docs currently don't say anything about error handling.

[soumyasen1809]

True...

https://github.com/image-rs/image/blob/40940d2a330b0c923481fc04fa06a852692224c4/src/imageops/mod.rs#L62-L78

Looking at the code, we can then replace iwidth and iheight like:

let x = cmp::min(x, iwidth-1); 
let y = cmp::min(y, iheight-1);