search

bkaradzic / bimg

Image library.
BSD 2-Clause "Simplified" License
327 stars 268 forks source link

Explicit sizes of the vector members in 3rd-party vectypes.h #54

Closed vladipus closed 3 years ago

vladipus commented 3 years ago

I have refactored a bit this (now seems obsolete) header to prevent buffer overflows.

bkaradzic commented 3 years ago

Can you describe how this buffer overflows happens and how this fixes it? It's not obvious.

vladipus commented 3 years ago

When you compile the code with sanity checks, the warnings are emitted in the console, stating that a buffer overrun will happen if the respective functions are called.

The problem is that the header in question seems ANCIENT. Somewhere in the distant past, the longs could really be 64 bits, but almost none use that convention currently, as longs are 32 bits under most compilers.

I have just altered the default types in the header to explicitly state the sizes of the ints. This way, there are no more warnings emitted regarding that issue.

I've noted that at least some of those functions are not used in bimg, but still the overflow warnings can be quite scary to see.

vladipus commented 3 years ago

Any help from me to merge the request?

bkaradzic commented 3 years ago

Can you fix just whatever is wrong, with long (change it to long long only in code that cause issue).

bkaradzic commented 3 years ago

You should also apply your changes in this repo: https://github.com/andrewwillmott/astc-encoder

vladipus commented 3 years ago

Can you fix just whatever is wrong, with long (change it to long long only in code that cause issue).

Would it be a better (more predictable) solution? My logic was that if the original code uses magic numbers relating to sizes, I should use the explicit bit types as well.

bkaradzic commented 3 years ago

Actually I don't mind your solution. The problem is where you submitted it. I want to minimize changes from upstream, so that I know what's going on. If you applied your change upstream, and they accepted it, I wouldn't mind just integrating it as-is here.

vladipus commented 3 years ago

You sure you have the latest version though? Anyway, I've created a PR: https://github.com/andrewwillmott/astc-encoder/pull/1 Hope the project is not abandoned...

bkaradzic commented 3 years ago

You sure you have the latest version though?

https://github.com/bkaradzic/bimg/commit/200165a0dd8517e32bc9a315e3f8e656ebd7d362

vladipus commented 3 years ago

Seems like the ASTC repo is stalled.