search

nothings / stb

stb single-file public domain libraries for C/C++
https://twitter.com/nothings
Other
26.31k stars 7.69k forks source link

stb_image: fix implicit conversion warnings #1443

Closed diamante0018 closed 1 year ago

diamante0018 commented 1 year ago

Hello, first-time contributor here. This pull request fixes 3 warnings that I get when using this library on the MSVC platform. I have set my MSVC compiler to treat these warnings as errors so this is necessary for me to continue using this library without making any changes to the solution file on visual studio/turning off warnings. Thanks.

Closes #1444

nothings commented 1 year ago

None of these seem like the correct way to fix the issue. For example, for security reasons, the code wants to check that multiplying two shorts gives a valid result. If one of the inputs isn't a short, but you cast it to a short for just that function call, you could truncate it smaller than it is, the security test passed, but then the actual multiply is done with a larger number, breaking the security check.

diamante0018 commented 1 year ago

So, if I understand this correctly, instead of casting you would prefer I clamp the value to be in between short min/max for that function call (And unsigned char for the final warning)? Is there a place in this repository where a similar thing like this is done already so I may have a look at it?

nothings commented 1 year ago

I'm pretty much saying that @rygorous (who wrote this code) should take a look at it and figure out what's going on. Presumably either dc ought to be stored in a short, or stbi__mul2shorts_valid should take ints, or something.

diamante0018 commented 1 year ago

Roger, feel free to close this pr when it is fixed properly or I will come back to it in a bit to see when it has been fixed to close it myself 🙂 I don't think I'm qualified enough to fix them myself but if the commit author wants to suggest anything I can do, I will try.

rygorous commented 1 year ago

I didn't write it, Neil did, but I'll have a look later.

diamante0018 commented 1 year ago

Thanks for the feedback, let me know if the last cast to unsigned char is fine. Also I'm not familiar with the code style of this project but I tried my best to follow it, please let me know if it is okay.

NBickford-NV commented 1 year ago

Thank you, that looks good to me from a security perspective - stbi__skip_jpeg_junk_at_end() always returns a value between 0 and 0xff inclusive, so another solution is it could return a stbi_uc instead, which would avoid the cast to unsigned char.

By the way, Sean and Fabian, please feel free to @ me any time - I'm always happy to help with things!

diamante0018 commented 1 year ago

You are right! I changed the return type of stbi__skip_jpeg_junk_at_end and removed the cast entirely. Thanks.

diamante0018 commented 1 year ago

Thanks for the tip, I will do that I think. Or just suppress the warning before including the headers.

ChrisThrasher commented 1 year ago

I'd like to bump this PR since I'm dealing with the same problems in #1444. @nothings Is this good to merge?

diamante0018 commented 1 year ago

Woot

rygorous commented 1 year ago

A fix for this is already in the dev branch (and has been since May 2nd, commit https://github.com/nothings/stb/commit/8c3aa0548766e6fef7da184107d35fd81315c1a5), it's just waiting on the next time we do a release.

diamante0018 commented 1 year ago

Better late then never. May I suggest that some MSVC tests are added to STB workflow(s) to see if it compiles with all warnings enabled. When I tried to compile stb (a week ago or so) it still failed.