Double free in Vtr::internal::StackBuffer when shrinking then destroying SurfaceData

[hgarrereyn]

Hi, there is a potential bug in SurfaceData reachable when resizing and then deleting.

This bug was reproduced on 9dab8a4.

Description

What crashes

• SurfaceData stores its control-vertex indices in a Vtr::internal::StackBuffer<Index,20,true> (inline capacity 20).
• Calling SurfaceData::resizeCVs(28) grows the StackBuffer above its inline capacity and allocates dynamic storage.
• Calling SurfaceData::resizeCVs(0) shrinks it below the inline capacity. StackBuffer::SetSize() calls deallocate(), which frees the dynamic block and switches back to the inline buffer.
• Deallocate() does not null _dynamicData, and the destructor of StackBuffer then calls deallocate() again unconditionally (or equivalently without guarding on current storage), leading to a double free of the same pointer.
• AddressSanitizer reports a double-free at vtr/stackBuffer.h:100 in both the original reproducer and the minimized testcase.

Note that removing the first resize (s->resizeCVs(28)) no longer crashes with a double free.

POC

The following testcase demonstrates the bug:

testcase.cpp

#include "/fuzz/install/include/opensubdiv/bfr/surfaceData.h"

int main(){
  using namespace OpenSubdiv::OPENSUBDIV_VERSION::Bfr::internal;
  SurfaceData *s = new SurfaceData();
  // Grow beyond inline capacity (SIZE = 20), which allocates dynamically
  s->resizeCVs(28);
  // Shrink to <= inline capacity, which frees the dynamic storage
  s->resizeCVs(0);
  // Destructor frees again: double-free
  delete s;
  return 0;
}

stdout

stderr

=================================================================
==1==ERROR: AddressSanitizer: attempting double-free on 0x50b000000040 in thread T0:
    #0 0x55f8cecb398d in operator delete(void*) (/fuzz/test+0x10598d) (BuildId: 22f0dd75076291483718a1eb9bd66c7bb72c2319)
    #1 0x55f8cecb56b1 in OpenSubdiv::v3_7_0::Vtr::internal::StackBuffer<int, 20u, true>::deallocate() /fuzz/install/include/opensubdiv/bfr/../vtr/stackBuffer.h:100:5
    #2 0x55f8cecb5894 in OpenSubdiv::v3_7_0::Vtr::internal::StackBuffer<int, 20u, true>::~StackBuffer() /fuzz/install/include/opensubdiv/bfr/../vtr/stackBuffer.h:165:5
    #3 0x55f8cecb5513 in OpenSubdiv::v3_7_0::Bfr::internal::SurfaceData::~SurfaceData() /fuzz/install/include/opensubdiv/bfr/surfaceData.h:38:36
    #4 0x55f8cecb5478 in main /fuzz/testcase.cpp:11:3
    #5 0x7f6b2c120d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #6 0x7f6b2c120e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #7 0x55f8cebda314 in _start (/fuzz/test+0x2c314) (BuildId: 22f0dd75076291483718a1eb9bd66c7bb72c2319)

0x50b000000040 is located 0 bytes inside of 112-byte region [0x50b000000040,0x50b0000000b0)
freed by thread T0 here:
    #0 0x55f8cecb398d in operator delete(void*) (/fuzz/test+0x10598d) (BuildId: 22f0dd75076291483718a1eb9bd66c7bb72c2319)
    #1 0x55f8cecb56b1 in OpenSubdiv::v3_7_0::Vtr::internal::StackBuffer<int, 20u, true>::deallocate() /fuzz/install/include/opensubdiv/bfr/../vtr/stackBuffer.h:100:5
    #2 0x55f8cecb5559 in OpenSubdiv::v3_7_0::Vtr::internal::StackBuffer<int, 20u, true>::SetSize(unsigned int) /fuzz/install/include/opensubdiv/bfr/../vtr/stackBuffer.h:192:9
    #3 0x55f8cecb54ce in OpenSubdiv::v3_7_0::Bfr::internal::SurfaceData::resizeCVs(int) /fuzz/install/include/opensubdiv/bfr/surfaceData.h:70:20
    #4 0x55f8cecb545d in main /fuzz/testcase.cpp:9:6
    #5 0x7f6b2c120d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

previously allocated by thread T0 here:
    #0 0x55f8cecb312d in operator new(unsigned long) (/fuzz/test+0x10512d) (BuildId: 22f0dd75076291483718a1eb9bd66c7bb72c2319)
    #1 0x55f8cecb5764 in OpenSubdiv::v3_7_0::Vtr::internal::StackBuffer<int, 20u, true>::allocate(unsigned int) /fuzz/install/include/opensubdiv/bfr/../vtr/stackBuffer.h:90:39
    #2 0x55f8cecb55c9 in OpenSubdiv::v3_7_0::Vtr::internal::StackBuffer<int, 20u, true>::SetSize(unsigned int) /fuzz/install/include/opensubdiv/bfr/../vtr/stackBuffer.h:195:9
    #3 0x55f8cecb54ce in OpenSubdiv::v3_7_0::Bfr::internal::SurfaceData::resizeCVs(int) /fuzz/install/include/opensubdiv/bfr/surfaceData.h:70:20
    #4 0x55f8cecb5452 in main /fuzz/testcase.cpp:7:6
    #5 0x7f6b2c120d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

SUMMARY: AddressSanitizer: double-free (/fuzz/test+0x10598d) (BuildId: 22f0dd75076291483718a1eb9bd66c7bb72c2319) in operator delete(void*)
==1==ABORTING

Steps to Reproduce

The crash was triaged with the following Dockerfile:

Dockerfile

# Ubuntu 22.04 with some packages pre-installed
FROM hgarrereyn/stitch_repro_base@sha256:3ae94cdb7bf2660f4941dc523fe48cd2555049f6fb7d17577f5efd32a40fdd2c

RUN git clone https://github.com/PixarAnimationStudios/OpenSubdiv /fuzz/src && \
    cd /fuzz/src && \
    git checkout 9dab8a47bfbb1388ec8388fe61f5f916e6123f38 && \
    git submodule update --init --remote --recursive

ENV LD_LIBRARY_PATH=/fuzz/install/lib
ENV ASAN_OPTIONS=hard_rss_limit_mb=1024:detect_leaks=0

RUN echo '#!/bin/bash\nexec clang-17 -fsanitize=address -O0 "$@"' > /usr/local/bin/clang_wrapper && \
    chmod +x /usr/local/bin/clang_wrapper && \
    echo '#!/bin/bash\nexec clang++-17 -fsanitize=address -O0 "$@"' > /usr/local/bin/clang_wrapper++ && \
    chmod +x /usr/local/bin/clang_wrapper++

# Install build dependencies
RUN apt-get update && apt-get install -y --no-install-recommends \
    ca-certificates cmake ninja-build make \
 && rm -rf /var/lib/apt/lists/*

# Configure and build OpenSubdiv (CPU-only, static libs)
ENV CC=clang_wrapper CXX=clang_wrapper++

WORKDIR /fuzz/src
RUN cmake -S . -B build \
    -G Ninja \
    -D CMAKE_BUILD_TYPE=Release \
    -D CMAKE_INSTALL_PREFIX=/fuzz/install \
    -D BUILD_SHARED_LIBS=OFF \
    -D NO_DOC=1 -D NO_EXAMPLES=1 -D NO_TUTORIALS=1 -D NO_REGRESSION=1 -D NO_PTEX=1 \
    -D NO_OMP=1 -D NO_TBB=1 -D NO_CUDA=1 -D NO_OPENCL=1 -D NO_CLEW=1 -D NO_OPENGL=1 -D NO_METAL=1 \
    -D NO_MACOS_FRAMEWORK=1
RUN cmake --build build --target install -- -v

Build Command

clang++-17 -fsanitize=address -g -O0 -o /fuzz/test /fuzz/testcase.cpp -I/fuzz/install/include -L/fuzz/install/lib -losdCPU && /fuzz/test

Reproduce

1. Copy Dockerfile and testcase.cpp into a local folder.
2. Build the repro image:

docker build . -t repro --platform=linux/amd64

3. Compile and run the testcase in the image:

docker run \
    -it --rm \
    --platform linux/amd64 \
    --mount type=bind,source="$(pwd)/testcase.cpp",target=/fuzz/testcase.cpp \
    repro \
    bash -c "clang++-17 -fsanitize=address -g -O0 -o /fuzz/test /fuzz/testcase.cpp -I/fuzz/install/include -L/fuzz/install/lib -losdCPU && /fuzz/test"

---

Additional Info

This testcase was discovered by STITCH, an autonomous fuzzing system. All reports are reviewed manually (by a human) before submission.

[davidgyu]

Filed as internal issue #OSD-478

[barfowl]

Thanks, Harrison. I was looking into this issue last week and it does reveal a bug. Your test case operates on internal rather than public classes (more on that later), so I'll clarify the impact on the public Bfr::Surface class here for those reading that are wondering how this may effect them...

In short, "typical" usage of Bfr::Surfaces with the SurfaceFactory (i.e. as illustrated in the tutorials and examples) will not encounter the problem described here, while use of Surface::operator=() to copy between instances may if the Surfaces are not initialized.

In more depth...

A precondition for the problem is that an instance of Surface to be reassigned has previously allocated dynamic memory for its CVs. Reassignment of such a Surface can then take place in one of two ways: either by the SurfaceFactory or by copying over that instance.

For the former, reassignment of a Surface via the SurfaceFactory should be safe as the factory always calls SurfaceData::resizeCVs() with a non-zero value, with the minimum being the number of vertices of the face. The number of CVs may increase and decrease but any dynamic memory is always retained -- only being released when replaced by a larger block when necessary.

But for the latter, a copy via Surface::operator=() may trigger the bug in certain circumstances. Here the destination Surface calls StackBuffer::SetSize() directly (not resizeCVs()), passing a zero size when the source Surface is uninitialized (i.e. default constructed -- initialized but empty and so not valid). This leads to the problem identified: freeing the dynamic memory in the destination while failing to reassign it, leading to the double free on destruction. This is easily reproduced in most of the tutorial examples by assigning an uninitialized Surface to the Surface used in the loop for all faces of a mesh after that loop -- provided one of the faces has triggered the dynamic allocation in the Surface.

The fix is trivial and I will start preparing a pull request. But I want to be clear in its description when and how the bug can be triggered, so I'm going to spend a bit more time investigating to be sure I haven't missed anything.

And thanks for the details in your post. It's not clear to me how STITCH operates, but as I mentioned earlier, your test case was making direct use of internal classes. Modifying such classes is not safe without an understanding of potential preconditions. So if you can't exclude them from STITCH's exercising/fuzzing, be prepared to filter its results (as I'm assuming you had to do here). Too bad STITCH didn't catch the Surface::operator=() bug more directly, but at least it got us there.

Thanks again