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
