zoneminder icon indicating copy to clipboard operation
zoneminder copied to clipboard

Misc fixes

Open dougnazar opened this issue 2 years ago • 6 comments

A few fixes I came across and some codeql cleanups.

dougnazar avatar Jul 31 '22 05:07 dougnazar

Damn bro, where did you come from? Lol.

You're really hitting the repo hard with fixes, good job and thank you!

baudneo avatar Jul 31 '22 15:07 baudneo

Regarding the: Cast operand to larger destination size to avoid possible overflow is

I kinda hate these random casts. I get why they might be needed, I'm just wondering if there is a nicer way to do it.

connortechnology avatar Aug 02 '22 14:08 connortechnology

For most of them looks like an inline function would probably work, silently promoting the operands. Not sure if it would work on 32bit for the first one, since that's specifically an unsigned long long.

size_t calculate_image_buffer_size(size_t width, size_t height, size_t colours);

dougnazar avatar Aug 02 '22 16:08 dougnazar

I guess what I am saying is that.... we already made imagesize an unsigned long long, presumably with the idea that that would fix the overflow problem. Your casting here seems to suggest that it doesn't. That we need to upgrade one of the operands to make it do 64bit multiplication before assigning to imagesize. We can be fairly sure for at least a few more years that width and height won't be very large, but we could just make them 64 integers too. I suppose another thought I have is that perhaps we should be using size_t instead of unsigned long long.

connortechnology avatar Aug 02 '22 17:08 connortechnology

Right, the issue (and warning) is that the calculation happens before the promotion. size_t should be fine on all platforms. that still handles 8k images on 32bit (4 bytes), and is 8 bytes on 64bit.

Using size_t for size, width, height, etc. should also fix the issue. If that's your preference, I can look at doing that. A quick grep shows only about 500 lines.

dougnazar avatar Aug 02 '22 17:08 dougnazar

Wouldn't it make sense to use the fixed-width typedefs ([u]int{8,16,32,64}) at least in those places where we don't want this platform-dependent ambiguity?

Carbenium avatar Aug 02 '22 19:08 Carbenium