stb_image: fix implicit conversion warnings #1443
Conversation
|
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. |
|
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 |
|
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 |
|
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 didn't write it, Neil did, but I'll have a look later. |
stb_image.h
Outdated
| @@ -2221,7 +2221,7 @@ static int stbi__jpeg_decode_block(stbi__jpeg *j, short data[64], stbi__huffman | |||
| if (!stbi__addints_valid(j->img_comp[b].dc_pred, diff)) return stbi__err("bad delta","Corrupt JPEG"); | |||
| dc = j->img_comp[b].dc_pred + diff; | |||
| j->img_comp[b].dc_pred = dc; | |||
| if (!stbi__mul2shorts_valid(dc, dequant[0])) return stbi__err("can't merge dc and ac", "Corrupt JPEG"); | |||
| if (!stbi__mul2shorts_valid((short) dc, dequant[0])) return stbi__err("can't merge dc and ac", "Corrupt JPEG"); | |||
There was a problem hiding this comment.
Oh yep, this was my fault in #1297 ! Thank you for the fix. How about checking that dc is valid to cast to a short, and casting the `stbi__uint16 as well, like this?
if ((dc > SHRT_MAX) || (dequant[0] > SHRT_MAX) || !stbi__mul2shorts_valid((short) dc, (short)dequant[0])) return stbi__err("can't merge dc and ac", "Corrupt JPEG");
stb_image.h
Outdated
| @@ -2278,7 +2278,7 @@ static int stbi__jpeg_decode_block_prog_dc(stbi__jpeg *j, short data[64], stbi__ | |||
| if (!stbi__addints_valid(j->img_comp[b].dc_pred, diff)) return stbi__err("bad delta", "Corrupt JPEG"); | |||
| dc = j->img_comp[b].dc_pred + diff; | |||
| j->img_comp[b].dc_pred = dc; | |||
| if (!stbi__mul2shorts_valid(dc, 1 << j->succ_low)) return stbi__err("can't merge dc and ac", "Corrupt JPEG"); | |||
| if (!stbi__mul2shorts_valid((short) dc, 1 << j->succ_low)) return stbi__err("can't merge dc and ac", "Corrupt JPEG"); | |||
There was a problem hiding this comment.
Similarly, maybe this could check as well that dc can be casted to a short before doing so, like this?
if ((dc > SHRT_MAX) || !stbi__mul2shorts_valid((short) dc, 1 << j->succ_low)) return stbi__err("can't merge dc and ac", "Corrupt JPEG");
|
Thanks for the feedback, let me know if the last cast to |
|
Thank you, that looks good to me from a security perspective - By the way, Sean and Fabian, please feel free to @ me any time - I'm always happy to help with things! |
|
You are right! I changed the return type of stbi__skip_jpeg_junk_at_end and removed the cast entirely. |
There was a problem hiding this comment.
The changes look good to me!
Ah, I don't have permission to merge anything in, though, and am not a project member, sorry - that would be @nothings and @rygorous . I understand it usually takes some time for things to be merged in - what I've usually done in the past for projects that require a patched version of stb_image is I've pointed Git submodules to the fork used for the merge request (or patched the copy if the project is using a copy of the file), then pointed them back to nothings:master once the patch has been merged.
|
Thanks for the tip, I will do that I think. Or just suppress the warning before including the headers. |
|
Woot |
|
A fix for this is already in the dev branch (and has been since May 2nd, commit 8c3aa05), it's just waiting on the next time we do a release. |
|
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. |
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