Added overflow handling to multiplication of width and components

[shikharvashistha]

Greetings of the day @fintelia,

I've fixed the issue as suggested, kindly review the pull and suggest changes if any.

Thanks & Regards
@shikharvashistha
Fixes : #1616

[5225225]

checked_mul returns a Option<usize>, which means it can fail.

There are multiple ways to solve this, but what I would suggest doing is something like

let row_size = match width.checked_mul(components) {
    Some(n) => n,
    None => return Err(<some error>),
};

Also, it's not entirely clear that using usize here is correct. It would be possible to change it to be usize everywhere (maybe, this doesn't look like it would affect the public API), but we use u32 everywhere else.

I'd keep it as a u32, and then you just need to deal with the error.

For the error... there doesn't look to be a good error to return already. So you'll want to make a new one.

To do that, you'll need to add a new enum variant to the DecoderError (it's at the top of pnm/decoder.rs). I'd do something like

/// The row was too wide and overflowed in multiplication
RowTooWide {
    width: u32,
    components: u32,
},

And then you'll need to add a human readable error for this. I'd say what the width and the components were, like

DecoderError::RowTooWide { width, components } =>
    f.write_fmt(format_args!("Row too wide: have width={}, components={}", width, components)),

And finally, I'd add a test. You can add the test at the bottom of pnm/decoder.rs, I'd look at how dimension_overflow works. Try and feed it the error file, and assert that it fails with your RowTooWide error.

---

Sorry about the wall of text! If anything's unclear, feel free to ask.

Also pinging @fintelia since i'm not sure if my suggestion to keep it as a u32 is right. But we seem to use u32 reasonably consistently for image dimensions elsewhere.

[fintelia]

We use u32 to store image dimensions quite frequently, but what concerns me here is that argument actually specifies the width of the image times the number of channels. I suggested changing to a usize in #1616 (comment) because we know that number will be at most equal to the size of buf, which by definition must fit into a usize.

[fintelia]

@shikharvashistha thanks for this PR! It isn't quite ready to merge yet, but we can work through some changes to get everything working.

First off, could you make sure you able to run cargo check and cargo test locally? Based on the Continious Integration tests, it seems that your changes don't compile, which is easiest to resolve by running the compiler locally and adjusting your code accordingly.

Next, I should perhaps provide more detailed directions. When I suggested changing the argument type, I mean changing line 201 so that from_bytes's second argument is row_size: usize:

image/src/codecs/pnm/decoder.rs

Line 201 in 0f1c38a

fn from_bytes(bytes: &[u8], row_size: u32, output_buf: &mut [u8]) -> ImageResult<()>;

Once you make that change, you'll see a couple compiler errors generated. These indicate places where other code has to be updated to get the types to match. Most of these should be straightforward to fix, but please ask questions if you get stuck.

[shikharvashistha]

Apologies for the delay I got busy in some university commitment, cargo check && cargo test seems to be passing/working for me now.

[HeroicKatora]

It seem like there is more panic after the changes, please review the code and see if the comments can help fix that.

[shikharvashistha]

Apologies, it's difficult for me to complete it at this time due to other time bound commitments, other's who are interested can take up the challenge.

[HeroicKatora]

Thanks for the notification, if you keep the branch and repository up then we (meaning anyone with write-access to this repository) can push further work into it. No problems at all, thanks for everything done already :)