Looks like imb_bmp_calc_row_size_in_bytes makes some hidden assumptions about actual data size (i.e. either 8bits, or 24bits), which should be documented in a comment I think? Or is it common, expected behavior with bmp files? At least it seems to differ from code in imb_bmp_decode itself when setting ibuf_depth?

Otherwise patch LGTM.

Looks like imb_bmp_calc_row_size_in_bytes makes some hidden assumptions about actual data size (i.e. either 8bits, or 24bits), which should be documented in a comment I think? Or is it common, expected behavior with bmp files?

This comes straight from Wikipedia's page about BMP.

One possible improvement/cleanup is to replace const int rowsize = (depth * x + 31) / 32 * 4; with the row_size_in_bytes.

At least it seems to differ from code in imb_bmp_decode itself when setting ibuf_depth

There seems to be a discrepancy between ibuf_depth and depth: the former denotes depth in the ImBuf, the latter one denotes how to access the pixels data, We are interested in the latter one.

@Bastien Montagne (mont29), did the cleanup. Should be no-functional changes. The change is indeed straightforward, but there is always a possibility to make a mistake in simple math. Hence extra eyes!

This patch still seems susceptible to various crashes. Attached are 2 more example files[2].

There was another patch[1] attached to the bug where I attempted a more complete fix but it needs updating a bit from recent changes. I can attempt to resurrect and re-test it this week if you like.

[1] D7945
[2] Examples

There are many missing checks in this patch indeed. You you start fuzzing the input, you'll discover even more.
Main goal was to tighten some nuts, avoiding obvious crashes. Also, keep in mind, if the change becomes too complex, it might be rather risky to put it into the 2.92 branch.

If you can resurrect your change and make it rock-solid please do it, that'd help a lot! I somehow missed the fact that you had a patch already, and there is no real need to duplicate the efforts.

For the time being, shall we go with more localized solution? I kind of feel we should put simpler/smaller patch to 2.92, solving actual user problem, and then put real complete fix from @Jesse Yurkovich (deadpin) into master. How people feel about this?

In D10357#260502, @Sergey Sharybin (sergey) wrote:

For the time being, shall we go with more localized solution? I kind of feel we should put simpler/smaller patch to 2.92, solving actual user problem, and then put real complete fix from @Jesse Yurkovich (deadpin) into master. How people feel about this?

Make sense to me, that kind of fix can totally be done incrementally.

Not sure if you were waiting for me but your plan makes sense. Checkin your fix for 2.92 and I'll get my patch ready for 2.93 etc.