Use size_t where appropriate and guard against overflow #1384
Conversation
zbjornson
left a comment
zbjornson
left a comment
There was a problem hiding this comment.
Thanks for picking this up! I think I said I'd work on it months ago and never did :(. Just a few questions.
(Github is being finicky. On the second comment, add: "Maybe consider keeping the members ints and just using size_t for the mul product?")
| jpeg_destroy_decompress(args); | ||
| this->errorInfo.set(NULL, "malloc", errno); | ||
| return CAIRO_STATUS_NO_MEMORY; | ||
| } |
There was a problem hiding this comment.
You could consider combining these two if bodies into one since you don't have to check overflow immediately after doing the mul.
| int width, height; | ||
| int naturalWidth, naturalHeight; | ||
| size_t width, height; | ||
| size_t naturalWidth, naturalHeight; |
There was a problem hiding this comment.
I'm trying to find any cases where int isn't big enough. The only one I found is reading a jpeg, where it's unsigned. GifLib, Cairo and librsvg all use int as far as I can tell. Was this type itself responsible for overflow?
One concern making this size_t is that width and height are passed directly to e.g. cairo_image_surface_create(format, int width, int height).
There was a problem hiding this comment.
If the image is 32798x32798 it will overflow and allocate way to little bytes since:
Good point about passing to cairo, I think that we already check for some specific value since cairo only accepts a maxiumum width or height of 2^15 - 1 (I think, ref:
Lines 610 to 613 in 3d81e0a
Okay, I just realized that the image I'm loading has a larger width & height than that, maybe we should just check that before even setting naturalWidht and naturalHeight at all 🤔
There was a problem hiding this comment.
I meant to propose: keep the members as int (or unsigned) since that's the largest Cairo can take, and only cast to a larger type where you multiply them and risk overflow.
| uint8_t *data = new uint8_t[naturalWidth * naturalHeight * 4]; | ||
|
|
||
| size_t stride = naturalWidth * 4; | ||
| if (naturalWidth != 0 && stride / naturalWidth != 4) { |
There was a problem hiding this comment.
We've got a host of these unchecked muls... might be worth copying a version of this function so it's easier to continue addressing them later.
https://github.com/nodejs/node/blob/master/src/util-inl.h#L297-304
(not sure if that link works, github is acting up. Line 297-304.)
|
Hmm, I don't know 😄 If Cairo is using |
|
Makes senses to me! 😃 |
2 participants
ping @zbjornson, @chearon
I think that
size_tmight be the correct type here since it's a "type which is used to represent sizes of objects in bytes", and "it is guaranteed to be big enough to contain the size of the biggest object that system can handle".