SDL_rect_impl.h:90:10: runtime error: signed integer overflow

[1bsyl]

following #7614, there are other overflows, and asan is seems to be a good tool to detect them.

here's a fuzzing testcase; updated .. Uploading main_bug_7614_clip_overflow_RANDOM.c.txt…

/src/video/SDL_rect_impl.h:90:10: runtime error: signed integer overflow: 2147483647 + 300 cannot be represented in type 'int'
/src/video/SDL_rect_impl.h:100:22: runtime error: signed integer overflow: -2147483349 - 2147483647 cannot be represented in type 'int'
/src/video/SDL_rect_impl.h:104:10: runtime error: signed integer overflow: 2147483647 + 2147483647 cannot be represented in type 'int'
/src/video/SDL_rect_impl.h:114:22: runtime error: signed integer overflow: -2 - 2147483647 cannot be represented in type 'int'
/src/video/SDL_blit.c:74:43: runtime error: signed integer overflow: 2147483647 * 4 cannot be represented in type 'int'
/src/video/SDL_blit.c:82:43: runtime error: signed integer overflow: 2147483647 * 4 cannot be represented in type 'int'
/src/video/SDL_blit_A.c:603:9: runtime error: signed integer overflow: 2147483647 + 3 cannot be represented in type 'int'

I haven't fixed that currently, maybe next week

[1bsyl]

I've updated the testcase.

I think there are potential overflow everywhere, in all SDL_rect.h functions, and probably more around. As soon as an operation is done with a big enough value, like INT_MAX you get the overflow.

• the thing is that most of these is non-sense. One doesn´t need a rect/point/width/viewport/ with INT_MAX value. I don't think we should go fixing them 1 by 1 one as soon as they appears, but change some internals with some convention, so that everything is solve the same way.

A few ideas:

• don't fix anything. (it's user task not to use non sense value, he will see when that crashes, like giving invalid pointers).

• any function that use rect, clip, w etc. should internally use Sint64. so pretty much: public API is "int32" but all internal local variables are sint64. but that doesn't really prevent to have overflow, once you put back a too big value to a SDL_rect.

• assume something like this for all SDL: we never compute more that 4 additions (maybe more). like "x + surface->width + clip.x + viewport.x" and limit any integer input to INT_MAX/4 (with something configurable...) (there're a few multiplications also, but there are less (hopefully), and this is could be another issue.. it's more like computing w * h).

[slouken]

There's a significant performance penalty to using 64-bit values on 32-bit systems, so we shouldn't convert everything to 64-bit. I've tried setting the limits to 16-bit and there are scenarios now where it's legitimate to try to work with surfaces larger than that.

One solution is to use 64-bits for intermediate calculations and then return an error if the final result overflows. That's been working fairly well in the places we've fixed up, but I haven't run any performance tests on those.

Maybe @smcv and @icculus have some insight here?

[1bsyl]

maybe more than 16 bits. A first generic constraint like 28 bits on each integer parameter would ok to protect additions from overflow.

then, there would remain the usual w * h constraints that shouldn't overlflow while creating surfaces.

[smcv]

Another possible approach would be to convert to size_t internally, and use SDL_size_mul_overflow() and SDL_size_add_overflow to reject ludicrous values that won't fit in the address space; or add a similar SDL_int_mul_overflow() and SDL_int_add_overflow. These are inlined and (are meant to) have well-defined behaviour for all arguments, and gcc implements them using CPU flags like the "previous calculation overflowed" bit where possible.

limit any integer input to INT_MAX/4

If not tracked very carefully, that sounds like a recipe for hidden assumptions that will accidentally be broken later. To me, it seems better to detect and prevent attempts to cause an overflow.

the thing is that most of these is non-sense. One doesn´t need a rect/point/width/viewport/ with INT_MAX value

On 32-bit machines, the memory buffer necessary to represent that geometry likely wouldn't even fit in addressable virtual memory (but on 64-bit, it could, because int is still only 32-bit signed in all ABIs that I know of).

[slouken]

add a similar SDL_int_mul_overflow() and SDL_int_add_overflow. These are inlined and (are meant to) have well-defined behaviour for all arguments, and gcc implements them using CPU flags like the "previous calculation overflowed" bit where possible.

Let's go ahead and implement this for SDL3.

[1bsyl]

I'm not sure this is a good idea, because all additions can do an overflow. So each addition would have to be moved to a SDL_int_add_overflow(), with an appropriate error handling.

• a lot of code to change and to add.
• not readable. this will add more bug, to fix a non-bug.
• doesn't clearly tell where the failure is.

You can have scenario where failure would occur lately: SDL_CreateSurface is ok SDL_SetClipRect is ok. SDL_BlitCopy or rendering fails. because adding the clip rect is added and is causing the overflow. so function fails, without telling the user a good recommendation of value.

Looking again:

• We're never accumulating more than 10 variables. so clearly 24 bits is fine. It's also +8000 x Full HD width. Largely enough for whatever I guess.

• the only multiplication to care is the render scale. where sometimes there are width * scale.
  So scale should also also be bounded. or maybe, since this are few place, this can be protected with SDL_mul_overflow

• there are height * pitch multiplication, but there are already overflow protected before doing allocations.

• indeed, pointer pixels + pitch could overflow easily on 32 bits machines.

[slouken]

Hmmmmmmm

[slouken]

I don't think we want to convert to entirely 64-bit math, and we don't want to add mul/add overflow functions everywhere, so I've added https://github.com/libsdl-org/SDL/pull/10336, which will help with the rectangle case. As you mentioned, surface allocation is also protected to validate pixels and pitch values.

I think in the other cases, there's no harm in the values overflowing, they'll just get nonsense values, usually clipped, for nonsense input. If anyone finds other cases that should be handled, feel free to open a new issue for those.