nothings / stb

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

remove potential UB in `stbiw__jpg_writeBits` #1617

Open Rexicon226 opened 7 months ago

Rexicon226 commented 7 months ago

The line:

https://github.com/nothings/stb/blob/ae721c50eaf761660b4f90cc590453cdb0c2acd0/stb_image_write.h#L1263

creates potential UB because int isn't guaranteed to be large enough. I think the solution is unsigned int but if you know something better please suggest.

You can see this easily yourself by just compiling a program like:

#define STB_IMAGE_IMPLEMENTATION
#include "stb_image.h"

#define STB_IMAGE_WRITE_IMPLEMENTATION
#include "stb_image_write.h"

int main(int argc, char const *argv[])
{
    char *filename = argv[1];
    int width, height, channels;
    unsigned char *img = stbi_load(filename, &width, &height, &channels, 0);

    stbi_write_jpg("temp.jpg", width, height, channels, img, 100);

    return 0;
}

Compile with:

clang -fsanitize=undefined test.c

running it:

stb_image_write.h:1264:14: runtime error: left shift of 12165120 by 8 places cannot be represented in type 'int'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior stb_image_write.h:1264:14 in
NBickford-NV commented 1 month ago

Hi! I worry a bit about the implicit unsigned-int to int conversion at the end of stbiw__jpg_writeBits() here:

   *bitBufP = bitBuf;

If the high bit of bitCnt is set and stb_image_write.h's compiled in C++ mode, then this was implementation-defined until C++20 (per bullet point 2 of https://en.cppreference.com/w/cpp/language/implicit_conversion#Integral_conversions) but worked OK anyways.

Maybe it's better to change the type of the bit buffer throughout stb_image_write.h to an unsigned int?

Since only the lower 24 bits of the bit buffer are used, I think this would work as well (at the expense of an additional mask):

   *bitBufP = bitBuf & 0xFFFFFF;

Thank you!