kinow commented on a change in pull request #196:
URL: https://github.com/apache/commons-imaging/pull/196#discussion_r790186878
##########
File path:
src/main/java/org/apache/commons/imaging/formats/bmp/BmpImageParser.java
##########
@@ -385,6 +385,10 @@ private BmpImageContents readImageContents(final
InputStream is,
+ bhi.compression);
}
+ if (paletteLength < 0) {
+ throw new ImageReadException("Invalid negative palette length: " +
paletteLength);
Review comment:
Good catch on the exception message prefix, thanks!
>Also, isn't the underlying issue that the colorTableSize is negative?
Shouldn't we check that instead?
I thought there would be a negative number before, so used the provided BMP
to test and noticed it was due to integer overflow when multiplying values.
>Hm, and that one is negative because bhi.bitsPerPixel is greater than
should be allowed, right?
`bhi.bitsPerPixel` is `8`, which looks OK.
The `BmpHeaderInfo` contains the number of colors used, and in this case is
`1667760130`, below the limit for Java integer. But when multiplied by `4` (due
to compression type) we get `6671040520`, causing the integer overflow.
>So the BmpHeaderInfo should not have been created in the first place? Maybe
that's where the error checking should really go.
Good question, I also wondered where would be the best place to put this
check. I couldn't figure out a way to prevent it from happening unless the
numbers were multiplied and then we could check if it's negative. That's why I
left the check here, below the `switch` statement, so that it covers all
possible branches checking for the integer overflow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
