nothings / stb

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

stb_image_resize2.h: Address Sanitizer error #1526

Closed pezcode closed 5 months ago

pezcode commented 9 months ago

Describe the bug

We're getting Address Sanitizer errors with stbir_resize() in stb_image_resize v2.00. Happens both with clang on Linux as well as MSVC. I added a full error log from an MSVC run further below.

To Reproduce

Compile and run this snippet with ASan enabled:

const int CHANNELS = 3;
char input[CHANNELS*3*2]{};
char output[CHANNELS*2*1]{};
stbir_resize(
    input, 3, 2, CHANNELS*3,
    output, 2, 1, CHANNELS*2,
    STBIR_RGB, STBIR_TYPE_UINT8, STBIR_EDGE_CLAMP, STBIR_FILTER_DEFAULT);

Expected behavior No ASan errors

Error log

==16616==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x1206efba9e68 at pc 0x7ffa6db4f045 bp 0x00eeb8ff9070 sp 0x00eeb8ff9070 READ of size 4 at 0x1206efba9e68 thread T0

0 0x7ffa6db4f044 in stbir__pack_coefficients C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:3700

1 0x7ffa6dbc4fef in stbir__alloc_internal_mem_and_build_samplers C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:6986

2 0x7ffa6dbc8d4b in stbir__perform_build C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7603

3 0x7ffa6db46150 in stbir_build_samplers_with_splits C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7639

4 0x7ffa6db457f7 in stbir_build_samplers C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7649

5 0x7ffa6db45c44 in stbir_resize_extended C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7666

6 0x7ffa6db43900 in stbir_resize C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7861

7 0x7ffa6dbc9f23 in Magnum::Trade::`anonymous namespace'::convertInternal C:\dev\fork\magnum-plugins\src\MagnumPlugins\StbResizeImageConverter\StbResizeImageConverter.cpp:66

8 0x7ffa6db417d1 in Magnum::Trade::StbResizeImageConverter::doConvert(class Magnum::ImageView<2, char const> const &) C:\dev\fork\magnum-plugins\src\MagnumPlugins\StbResizeImageConverter\StbResizeImageConverter.cpp:215

9 0x7ffa6dc1e514 in Magnum::Trade::AbstractImageConverter::convert(class Magnum::ImageView<2, char const> const &) C:\dev\fork\magnum\src\Magnum\Trade\AbstractImageConverter.cpp:176

10 0x7ff656c834ee in Magnum::Trade::Test::`anonymous namespace'::StbResizeImageConverterTest::emptySize C:\dev\fork\magnum-plugins\src\MagnumPlugins\StbResizeImageConverter\Test\StbResizeImageConverterTest.cpp:180

11 0x7ffb789299a9 in Corrade::TestSuite::Tester::exec(class Corrade::TestSuite::Tester , class std::basic_ostream<char, struct std::char_traits> , class std::basic_ostream<char, struct std::char_traits> *) C:\dev\fork\corrade\src\Corrade\TestSuite\Tester.cpp:571

12 0x7ffb78925483 in Corrade::TestSuite::Tester::exec(void) C:\dev\fork\corrade\src\Corrade\TestSuite\Tester.cpp:238

13 0x7ff656c91cec in main C:\dev\fork\magnum-plugins\src\MagnumPlugins\StbResizeImageConverter\Test\StbResizeImageConverterTest.cpp:493

14 0x7ff656cb56b7 in wmain C:\dev\fork\corrade\src\Corrade\CorradeMain.cpp:138

15 0x7ff656cafb08 in invoke_main D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:90

16 0x7ff656cafa2d in __scrt_common_main_seh D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288

17 0x7ff656caf8ed in __scrt_common_main D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:330

18 0x7ff656cafb7d in wmainCRTStartup D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_wmain.cpp:16

19 0x7ffbad6f257c (C:\WINDOWS\System32\KERNEL32.DLL+0x18001257c)

20 0x7ffbae82aa67 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x18005aa67)

0x1206efba9e68 is located 8 bytes to the left of 16-byte region [0x1206efba9e70,0x1206efba9e80) allocated by thread T0 here:

0 0x7ffa6b1d3c18 in malloc D:\a_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\asan\asan_malloc_win.cpp:252

1 0x7ffa6dbc45b6 in stbir__alloc_internal_mem_and_build_samplers C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:6939

2 0x7ffa6dbc8d4b in stbir__perform_build C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7603

3 0x7ffa6db46150 in stbir_build_samplers_with_splits C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7639

4 0x7ffa6db457f7 in stbir_build_samplers C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7649

5 0x7ffa6db45c44 in stbir_resize_extended C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7666

6 0x7ffa6db43900 in stbir_resize C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:7861

7 0x7ffa6dbc9f23 in Magnum::Trade::`anonymous namespace'::convertInternal C:\dev\fork\magnum-plugins\src\MagnumPlugins\StbResizeImageConverter\StbResizeImageConverter.cpp:66

8 0x7ffa6db417d1 in Magnum::Trade::StbResizeImageConverter::doConvert(class Magnum::ImageView<2, char const> const &) C:\dev\fork\magnum-plugins\src\MagnumPlugins\StbResizeImageConverter\StbResizeImageConverter.cpp:215

9 0x7ffa6dc1e514 in Magnum::Trade::AbstractImageConverter::convert(class Magnum::ImageView<2, char const> const &) C:\dev\fork\magnum\src\Magnum\Trade\AbstractImageConverter.cpp:176

10 0x7ff656c834ee in Magnum::Trade::Test::`anonymous namespace'::StbResizeImageConverterTest::emptySize C:\dev\fork\magnum-plugins\src\MagnumPlugins\StbResizeImageConverter\Test\StbResizeImageConverterTest.cpp:180

11 0x7ffb789299a9 in Corrade::TestSuite::Tester::exec(class Corrade::TestSuite::Tester , class std::basic_ostream<char, struct std::char_traits> , class std::basic_ostream<char, struct std::char_traits> *) C:\dev\fork\corrade\src\Corrade\TestSuite\Tester.cpp:571

12 0x7ffb78925483 in Corrade::TestSuite::Tester::exec(void) C:\dev\fork\corrade\src\Corrade\TestSuite\Tester.cpp:238

13 0x7ff656c91cec in main C:\dev\fork\magnum-plugins\src\MagnumPlugins\StbResizeImageConverter\Test\StbResizeImageConverterTest.cpp:493

14 0x7ff656cb56b7 in wmain C:\dev\fork\corrade\src\Corrade\CorradeMain.cpp:138

15 0x7ff656cafb08 in invoke_main D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:90

16 0x7ff656cafa2d in __scrt_common_main_seh D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288

17 0x7ff656caf8ed in __scrt_common_main D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:330

18 0x7ff656cafb7d in wmainCRTStartup D:\a_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_wmain.cpp:16

19 0x7ffbad6f257c (C:\WINDOWS\System32\KERNEL32.DLL+0x18001257c)

20 0x7ffbae82aa67 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x18005aa67)

SUMMARY: AddressSanitizer: heap-buffer-overflow C:\dev\fork\magnum-plugins\src\external\stb\stb_image_resize2.h:3700 in stbir__pack_coefficients Shadow bytes around the buggy address: 0x0443cdaf5370: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00 0x0443cdaf5380: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00 0x0443cdaf5390: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00 0x0443cdaf53a0: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00 0x0443cdaf53b0: fa fa 00 00 fa fa 00 00 fa fa 00 00 fa fa 00 00 =>0x0443cdaf53c0: fa fa 00 00 fa fa 00 00 fa fa 00 07 fa[fa]00 00 0x0443cdaf53d0: fa fa 00 fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0443cdaf53e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0443cdaf53f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0443cdaf5400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0443cdaf5410: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa Shadow byte legend (one shadow byte represents 8 application bytes): Addressable: 00 Partially addressable: 01 02 03 04 05 06 07 Heap left redzone: fa Freed heap region: fd Stack left redzone: f1 Stack mid redzone: f2 Stack right redzone: f3 Stack after return: f5 Stack use after scope: f8 Global redzone: f9 Global init order: f6 Poisoned by user: f7 Container overflow: fc Array cookie: ac Intra object redzone: bb ASan internal: fe Left alloca redzone: ca Right alloca redzone: cb

jeffatrad commented 9 months ago

I'll take a look this weekend - I asaned on Windows, maybe missed something. Maybe it's just something with parameters so tiny...

jeffatrad commented 9 months ago

In the meantime, can you send my your compile command line so I can match exactly?

pezcode commented 9 months ago

Thanks for looking into it 😌

Not 100% sure which of these options are relevant, so I attached the entire MSVC 2022 command line.

If it makes things simpler, this a completely default MSVC project generated by CMake with the only change being /fsanitize=address:

project(stbir2-asan-test CXX)
add_executable(${PROJECT_NAME} main.cpp)
target_compile_options(${PROJECT_NAME} PRIVATE -fsanitize=address)

If you add the repro code up top to main.cpp and copy + include stb_image_resize2.h, you should be good.

Anyway, complete command lines:

Compiler:

/ifcOutput "stbir2-asan-test.dir\Debug\" /GS /W3 /Zc:wchar_t /Zi /Gm- /Od /Ob0 /Fd"stbir2-asan-test.dir\Debug\vc143.pdb" /Zc:inline /fp:precise /D "_MBCS" /D "WIN32" /D "_WINDOWS" /D "CMAKE_INTDIR=\"Debug\"" /errorReport:prompt /fsanitize=address /WX- /Zc:forScope /RTC1 /GR /Gd /MDd /Fa"stbir2-asan-test.dir\Debug\" /EHsc /nologo /Fo"stbir2-asan-test.dir\Debug\" /Fp"stbir2-asan-test.dir\Debug\stbir2-asan-test.pch" /diagnostics:column

Linker:

/OUT:"C:\dev\stbir2-asan\build\Debug\stbir2-asan-test.exe" /MANIFEST /NXCOMPAT /PDB:"C:/dev/stbir2-asan/build/Debug/stbir2-asan-test.pdb" /DYNAMICBASE "kernel32.lib" "user32.lib" "gdi32.lib" "winspool.lib" "shell32.lib" "ole32.lib" "oleaut32.lib" "uuid.lib" "comdlg32.lib" "advapi32.lib" /IMPLIB:"C:/dev/stbir2-asan/build/Debug/stbir2-asan-test.lib" /DEBUG /MACHINE:X64 /INCREMENTAL /PGD:"C:\dev\stbir2-asan\build\Debug\stbir2-asan-test.pgd" /SUBSYSTEM:CONSOLE /MANIFESTUAC:"level='asInvoker' uiAccess='false'" /ManifestFile:"stbir2-asan-test.dir\Debug\stbir2-asan-test.exe.intermediate.manifest" /LTCGOUT:"stbir2-asan-test.dir\Debug\stbir2-asan-test.iobj" /ERRORREPORT:PROMPT /ILK:"stbir2-asan-test.dir\Debug\stbir2-asan-test.ilk" /NOLOGO /TLBID:1

NBickford-NV commented 9 months ago

In case it helps, it looks like the bug is on line 3700, where the loop looks like this:

    stbir__contributors * contribs = contributors + num_contributors - 1;
    float * coeffs = coefficents + widest * ( num_contributors - 1 );

    // go until no chance of clipping (this is usually less than 8 lops)
    while ( ( ( contribs->n0 + widest*2 ) >= row_width ) && ( contribs >= contributors ) )
    {
      ...
      --contribs;
      coeffs -= widest;
    }

contribs iterates backwards through coefficients; in pezcode's example above, contribs reaches the 8 bytes before the start of contributors, and then the first clause in the while condition tries to dereference contribs->n0.

Since there's a range check later on, I think the fix is to swap the first and second clauses in the condition so short-circuit evaluation works correctly:

    // go until no chance of clipping (this is usually less than 8 lops)
    while ( (contribs >= contributors) && ( ( contribs->n0 + widest*2 ) >= row_width ) )
jeffatrad commented 9 months ago

Yeah, I think it's something simple like that - I'll make sure and get it updated.

pezcode commented 9 months ago

Since there's a range check later on, I think the fix is to swap the first and second clauses in the condition so short-circuit evaluation works correctly:

    // go until no chance of clipping (this is usually less than 8 lops)
    while ( (contribs >= contributors) && ( ( contribs->n0 + widest*2 ) >= row_width ) )

Can confirm that this fixes the ASan error for me👍

jeffatrad commented 9 months ago

Confirmed that this is the correct fix, will be posting fix soon. Only triggers with asan, since we use one big allocation in non-asan and it would just do the single bad read from the internal scanline buffer, heh. Also, tested all small input sizes less than 16 (most of my asan testing was on small output buffers).

pezcode commented 5 months ago

Fix from https://github.com/nothings/stb/pull/1561 made it into 2.04, can confirm that fixes the issue. Thanks!