[BUG] ASAN heap-use-after-free while concurrently opening PSD files

[jessey-git]

Describe the bug
See below for the full ASAN output but it seems like the ImageCache might not be threadsafe in this particular instance maybe?

To Reproduce
Steps to reproduce the behavior:

1. Concurrently .open() the 8 .PSD files from the oiio-images repo
2. ASAN will complain while processing the resources in the PSDInput::load_resource_thumbnail code path (relating to the ImageCache and copying of ImageSpecs)

Expected behavior
No crash

Evidence
oiio-asan.txt

If I disable threading within the application, so it only loads 1 .PSD at a time, things seem ok.

I haven't been able to nail down the cause. Any ideas? I can attempt a stand alone repro sometime tomorrow maybe and will try to help out as best I can.

Platform information:

• OIIO branch/version: 2.4.9
• OS: Linux
• C++ compiler: 11.3
• Any non-default build flags when you build OIIO:

[jessey-git]

The following snippet is sufficient for me to repro a variety of oddness stemming from that load_resource_thumbnail code path.

    std::vector<std::string> files;
    files.emplace_back("<your path>/oiio-images/psd_123.psd");
    files.emplace_back("<your path>/oiio-images/psd_123_nomaxcompat.psd");
    files.emplace_back("<your path>/oiio-images/psd_bitmap.psd");
    files.emplace_back("<your path>/oiio-images/psd_indexed_trans.psd");
    files.emplace_back("<your path>/oiio-images/psd_rgb_8.psd");
    files.emplace_back("<your path>/oiio-images/psd_rgb_16.psd");
    files.emplace_back("<your path>/oiio-images/psd_rgb_32.psd");
    files.emplace_back("<your path>/oiio-images/psd_rgba_8.psd");

    std::vector<std::thread> threads;
    threads.reserve(files.size());

    for (const std::string& file : files) {
        threads.push_back(std::thread([file] {
            for (int i = 0; i < 100; i++) {
                unique_ptr<ImageInput> in = ImageInput::create("psd");
                ImageSpec newspec, config;
                if (in->open(file, newspec, config)) {
                    printf("success for %s\n", file.c_str());
                    in->close();
                }
                else {
                    printf("failure to open %s\n", file.c_str());
                }
            }
            }));
    }

    for (auto& thread : threads) {
        thread.join();
    }

[lgritz]

I have some PSD overhaul in progress, see #3807

Before I dig into the current issue, would you please see if you are able to pull that patch and determine if the thread issue is still a problem with those proposed changes?

[jessey-git]

Sadly, yes, the issue is there after applying your change. I had to switch back to Windows for compiling but it's also firing ASAN hits in the code path (different than before fwiw).

==9508==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x1227bc313640 at pc 0x7ffd831241fc bp 0x00a7f5aff020 sp 0x00a7f5afe7a0
WRITE of size 57600 at 0x1227bc313640 thread T6
    #0 0x7ffd83124229 in _asan_wrap_GlobalSize+0x413d9 (C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x180044229)
    #1 0x7ff797552a8c in OpenImageIO_v2_5_1::convert_pixel_values+0x33c (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x140042a8c)
    #2 0x7ff7975486ef in OpenImageIO_v2_5_1::ImageInput::read_scanlines+0x50f (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x1400386ef)
    #3 0x7ff797546b32 in OpenImageIO_v2_5_1::ImageInput::read_image+0x542 (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x140036b32)
    #4 0x7ff79764e204 in OpenImageIO_v2_5_1::ImageBufImpl::read+0xb14 (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x14013e204)
    #5 0x7ff79764d6e2 in OpenImageIO_v2_5_1::ImageBuf::read+0x72 (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x14013d6e2)
    #6 0x7ff7975a66de in OpenImageIO_v2_5_1::PSDInput::load_resource_thumbnail+0x31e (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x1400966de)
    #7 0x7ff7975a6e0c in OpenImageIO_v2_5_1::PSDInput::load_resources+0x36c (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x140096e0c)
    #8 0x7ff7975a7065 in OpenImageIO_v2_5_1::PSDInput::open+0x155 (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x140097065)
    #9 0x7ff797515227 in `main'::`6'::<lambda_1>::operator() C:\Users\jesse\source\OIIOTest\OIIOTest.cpp:69
    #10 0x7ff79751732d in std::thread::_Invoke<std::tuple<`main'::`6'::<lambda_1> >,0> C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\include\thread:55
    #11 0x7ffe4c2e1bb1 in configthreadlocale+0x91 (C:\WINDOWS\System32\ucrtbase.dll+0x180021bb1)
    #12 0x7ffd831417b9 in _asan_wrap_GlobalSize+0x5e969 (C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x1800617b9)
    #13 0x7ffe4e997603 in BaseThreadInitThunk+0x13 (C:\WINDOWS\System32\KERNEL32.DLL+0x180017603)
    #14 0x7ffe4eb826a0 in RtlUserThreadStart+0x20 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x1800526a0)

0x1227bc313640 is located 0 bytes to the right of 37440-byte region [0x1227bc30a400,0x1227bc313640)
allocated by thread T6 here:
    #0 0x7ff7976e14b3 in operator new[] D:\a\_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\asan\asan_win_new_array_thunk.cpp:42
    #1 0x7ff79764cf7e in OpenImageIO_v2_5_1::ImageBufImpl::new_pixels+0x4e (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x14013cf7e)
    #2 0x7ff79764e3f0 in OpenImageIO_v2_5_1::ImageBufImpl::realloc+0x40 (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x14013e3f0)
    #3 0x7ff79764dde4 in OpenImageIO_v2_5_1::ImageBufImpl::read+0x6f4 (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x14013dde4)
    #4 0x7ff79764d6e2 in OpenImageIO_v2_5_1::ImageBuf::read+0x72 (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x14013d6e2)
    #5 0x7ff7975a66de in OpenImageIO_v2_5_1::PSDInput::load_resource_thumbnail+0x31e (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x1400966de)
    #6 0x7ff7975a6e0c in OpenImageIO_v2_5_1::PSDInput::load_resources+0x36c (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x140096e0c)
    #7 0x7ff7975a7065 in OpenImageIO_v2_5_1::PSDInput::open+0x155 (C:\Users\jesse\source\OIIOTest\x64\Release\OIIOTest.exe+0x140097065)
    #8 0x7ff797515227 in `main'::`6'::<lambda_1>::operator() C:\Users\jesse\source\OIIOTest\OIIOTest.cpp:69
    #9 0x7ff79751732d in std::thread::_Invoke<std::tuple<`main'::`6'::<lambda_1> >,0> C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\include\thread:55
    #10 0x7ffe4c2e1bb1 in configthreadlocale+0x91 (C:\WINDOWS\System32\ucrtbase.dll+0x180021bb1)
    #11 0x7ffd831417b9 in _asan_wrap_GlobalSize+0x5e969 (C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x1800617b9)
    #12 0x7ffe4e997603 in BaseThreadInitThunk+0x13 (C:\WINDOWS\System32\KERNEL32.DLL+0x180017603)
    #13 0x7ffe4eb826a0 in RtlUserThreadStart+0x20 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x1800526a0)

Thread T6 created by T0 here:
    #0 0x7ffd83142ecf in _asan_wrap_GlobalSize+0x6007f (C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.34.31933\bin\HostX64\x64\clang_rt.asan_dynamic-x86_64.dll+0x180062ecf)
    #1 0x7ffe4c2e1896 in beginthreadex+0x56 (C:\WINDOWS\System32\ucrtbase.dll+0x180021896)
    #2 0x7ff797514cdc in main C:\Users\jesse\source\OIIOTest\OIIOTest.cpp:65
    #3 0x7ff7976e1d6b in __scrt_common_main_seh D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288
    #4 0x7ffe4e997603 in BaseThreadInitThunk+0x13 (C:\WINDOWS\System32\KERNEL32.DLL+0x180017603)
    #5 0x7ffe4eb826a0 in RtlUserThreadStart+0x20 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x1800526a0)

[lgritz]

Ok, thanks, I will dig into this.

[jessey-git]

We may have users running into this now so I've taken another look. Unfortunately I haven't quite root caused it but it does seem related to the ImageCache. If I change PSDInput::load_resource_thumbnail to use a unique name, rather than thumbnail.jpg, things are able to run to completion. It's like everyone is colliding on that single name.

While I suspect this name collision to be logically incorrect, and it should probably be changed, that still leaves the question of why such a collision yields crashes.

[lgritz]

Oh, I see, yes. If the ImageBufs are underneath using the ImageCache as an intermediary (including for storing the spec and metadata), we are violating a basic IC assumption that reading one named image twice should yield the same results! I'm not sure that the ImageCache per se is not thread safe, but the way we are using it in this spot seems very unwise -- we are inherently setting up several reads simultaneously using the same filename but that certainly represent different images (possibly having different dimensions, I bet that's what really hurts).

I think that the best solution is just to give that ImageBuf a unique name so they can't conflict. That's simple, I will do that right now and submit a PR.

In the longer run, I've been thinking for quite a while that the rules for when an IB uses IC as backing, or not, is complicated and hard to reason about, and I have been contemplating a refactor where IB by default does not use IC underneath, unless the IB is constructed (or reset) with a pointer to an IC to use. That is, leave it totally to the caller to determine if they want an IC-backed IB, and for those that are not IC-back, really really be not IC-backed and have no hidden implicit use of IC. But let's tackle that as a separate, later step.

[lgritz]

Proposed fix here: #3877