Address Sanitizer on Windows reports a buffer overflow in `Halide::serialize_pipeline`

[shoaibkamil]

An internal Adobe user reports that running a generator executable results in a container-overflow in the serialization code. Specifically, the offending code appears to be in Halide::Internal::Serializer::serialize(class Halide::Pipeline const &, class std::vector<unsigned char, class std::allocator<unsigned char>> &).

[steven-johnson]

A repro case would help :-)

[abadams]

Some more detail on the complaint:

1>==16204==ERROR: AddressSanitizer: container-overflow on address 0x021081271820 at pc 0x7ff9c3819a86 bp 0x00f43c98a4c0 sp 0x00f43c989c50
1>WRITE of size 117776 at 0x021081271820 thread T0
1>    #0 0x7ff9c3819a85 in __asan_wrap_memmove D:\a\_work\1\s\src\vctools\asan\llvm\compiler-rt\lib\sanitizer_common\sanitizer_common_interceptors.inc:813
1>    #1 0x7ff991b286ad in std::vector<unsigned char, class std::allocator<unsigned char>>::_Insert_range<unsigned char *>(class std::_Vector_const_iterator<class std::_Vector_val<struct std::_Simple_types<unsigned char>>>, unsigned char *, unsigned char *, struct std::forward_iterator_tag) 

[abadams]

It looks like it has to be the insert call here:

    uint8_t *buf = builder.GetBufferPointer();
    int size = builder.GetSize();

    if (buf != nullptr && size > 0) {
        result.clear();
        result.reserve(size);
        result.insert(result.begin(), buf, buf + size);
    } else {
        user_error << "failed to serialize pipeline!\n";
    }

But I don't see what could be wrong with it. It's a bit weird to use begin() instead of end(), but the spec says those are equal for an empty container.

[shoaibkamil]

A repro case would help :-)

Agreed, we'll try to create one. I concur with @abadams that it's not immediately obvious what's wrong. @slomp is also looking into this.

[abadams]

It's likely this is a false positive. I believe in this case Halide.dll was compiled without asan, but the generator was compiled with asan, and the std::vector in question is one that was created in the generator and passed into Halide.dll to be resized and filled. When Halide resizes it, it probably doesn't set up the right asan tracking state to catch overflows.

[slomp]

This diff adds a new interface to workaround the issue. Now, the question is: is it worth? Halide_patch.diff.txt

[abadams]

I think this is just a complex case of: don't allocate memory in a dll and free it in an exe (or vice versa), because those may be separate heaps with separate settings. I know there are cases where we deliberately avoided doing this. Maybe there's some way to catch all such cases instead of playing whack-a-mole. Passing stl types by reference definitely seems problematic on windows - we could forbid that, but without testing it's going to creep back in.

The cleanest workaround I think is to change it to just return a pimpl intrusive pointer type like Halide::Buffer, so that no reallocations happen in the exe, and the constructor and destructor both happen in the dll

[slomp]

I like the idea of returning a Halide::Buffer. We could perhaps offer a type alias for that buffer for the (de)serialization interface(s).

There may be other places where this problem is prone to happen (std::string modifications come to mind).

[slomp]

Ok, so there are these special compiler directives to enable some intrusive STL container checks alongside ASAN: https://blog.trailofbits.com/2024/09/10/sanitize-your-c-containers-asan-annotations-step-by-step/

For the most part, they are opt-in by default (clang, gcc); however for MSVC, they are opt-out.

I think we don't need to take any action and we can close the ticket. If someone is linking against libraries that requires one to pass STL containers around, the right thing to do when building the host executable with ASAN is to make sure these STL annotations are disabled (or rebuild said libraries with ASAN and STL annotations).

[slomp]

Should we close it?

[abadams]

Not sure. Are we trying avoid allocating memory in Halide.dll and freeing it outside Halide.dll (or vice-versa)? The handling of custom lowering passes indicates that we are, in which case this API does need fixing.

[slomp]

Not sure. Are we trying avoid allocating memory in Halide.dll and freeing it outside Halide.dll (or vice-versa)? The handling of custom lowering passes indicates that we are, in which case this API does need fixing.

Sure, but that has a much bigger scope. A new ticket issue that references this one, maybe?

[abadams]

Actually I think it's a fool's errand, because we have a bunch of places where we return a std::vector, and I think calling any of those would allocate inside of the dll and free in the exe.