Hi netsurf-dev,
I've had a look at the recently broken off BMP loading library
libnsbmp from the point of view of robustness against malformed
images, and today finally created some test images to exercise
the issues I've found. Mostly the library seems okay, except
perhaps a bit too trusting of the input. As to its design, I
really like the approach of shuffling off the image allocation to
the client, as that puts the client in control of the main
resource usage. Could do with some bounds checked arithmetic and
array access primitives though.
Most of the potential out of bounds data writes in the code were
cunningly foiled by a restriction to 16 bits of width/height in
the library's bmp struct, so whoever thought of that deserves
kudos. :) Still, a few potential overflows remain, and even more
if the client is careless about how they allocate the image
bitmap, so to avoid them all I suggest restricting acceptable
widths and heights to be less than 16k.
I've gathered single examples of the problems I've found in the
bmp_<foo>() functions. There's some code and logic duplication
between different parts of libnsbmp, so the same problem
sometimes appears in multiple places, but I haven't gone and made
tests cases for each of those as they look all the same of
course.
The images themselves along with source, memcheck reports and
some further explanations are here:
http://www.discontinuity.info/~rowan/libnsbmp-tests.tar.gz
Those don't cover any of the ico/image collection parsing code,
but there seemed to be some issues there as well. Things like
trusting bitmap offset and such, but I haven't made tests for
those.
Finally, and totally tangentially to the test images, could I
make a feature request regarding the client callbacks? The
current set of callbacks includes a function
bmp_bitmap_cb_get_bpp bitmap_get_bpp; /**< Find the width of a pixel row in bytes. */
which seems quite odd since libnsbmp treats the return value as a
bytes_per_pixel value (which always *must* be four or bad things
will happen) instead of row stride / pitch like the comment
suggests. It would be useful if there was actually a callback
for the row stride.
Cheers,
Joonas Pihlaja