Crash on invalid array access in mbgl::style::Collection::update

[springmeyer]

Platform: Seen on Linux + OS X with a node API
Mapbox SDK version: Latest (ec12282)

Steps to trigger behavior

1. Define a custom Filesource that runs synchronously and returns assets instantly from a pre-loaded in-memory <unordered_map>
2. Load a style with sources
3. Filesource is called and returns valid data for source immediately
4. Crash happens when source is loaded during style load

The custom filesource looks like:

class InMemoryFileSource : public mbgl::FileSource {
  public:
    std::unique_ptr<mbgl::AsyncRequest> request(const mbgl::Resource& resource,
                                                mbgl::FileSource::Callback callback) override {
        mbgl::Response response;
        auto it = assets.find(resource.url);
        if (it == assets.end()) {
            response.error = std::make_unique<mbgl::Response::Error>(mbgl::Response::Error::Reason::NotFound, std::string{"Not Found: "} + resource.url);
        } else {
            response.data = it->second;
        }
        callback(response);
        return nullptr;
    }

    void add(std::string&& key, char* buffer, std::size_t size) {
        assets.emplace(std::move(key), std::make_shared<std::string>(buffer, size));
    };

  private:
    std::unordered_map<std::string, std::shared_ptr<std::string>> assets;
};

I'm replicating with this style: https://github.com/mapbox/mapbox-gl-styles/blob/master/styles/bright-v9.json

Expected behavior

No crash on invalid out of range vector access so that the style loads successfully.

Actual behavior

Crash in both release and debug mode is:

libc++abi.dylib: terminating with uncaught exception of type std::out_of_range: vector

The problem here is not the uncaught exception (that is issue I can fix in my code).

The problem is the std::out_of_range: vector which may be a bug in core.

Full backtrace (in debug mode) is:

 thread #15, stop reason = signal SIGSTOP
    frame #0: 0x00007fff7ef9d2c6 libsystem_kernel.dylib`__pthread_kill + 10
    frame #1: 0x00007fff7f058bf1 libsystem_pthread.dylib`pthread_kill + 284
    frame #2: 0x00007fff7ef076a6 libsystem_c.dylib`abort + 127
    frame #3: 0x00007fff7c0e3641 libc++abi.dylib`abort_message + 231
    frame #4: 0x00007fff7c0e37c7 libc++abi.dylib`default_terminate_handler() + 243
    frame #5: 0x00007fff7d696eeb libobjc.A.dylib`_objc_terminate() + 105
    frame #6: 0x00007fff7c0ef19e libc++abi.dylib`std::__terminate(void (*)()) + 8
    frame #7: 0x00007fff7c0eef86 libc++abi.dylib`__cxxabiv1::failed_throw(__cxxabiv1::__cxa_exception*) + 27
    frame #8: 0x00007fff7c0e1f99 libc++abi.dylib`__cxa_throw + 113
    frame #9: 0x00007fff7c0bd2eb libc++.1.dylib`std::__1::__vector_base_common<true>::__throw_out_of_range() const + 71
    frame #10: 0x00000001053bbdce mbgl.node`std::__1::vector<mbgl::Immutable<mbgl::style::Source::Impl>, std::__1::allocator<mbgl::Immutable<mbgl::style::Source::Impl> > >::at(this=<unavailable>, __n=<unavailable>) at vector:1561:15 [opt]
    frame #11: 0x00000001053bbd5e mbgl.node`auto mbgl::style::Collection<mbgl::style::Source>::update(this=<unavailable>, impls_=size=0)::'lambda'(auto&)::operator()<std::__1::vector<mbgl::Immutable<mbgl::style::Source::Impl>, std::__1::allocator<mbgl::Immutable<mbgl::style::Source::Impl> > > >(auto&) const at collection.hpp:136:16 [opt]
    frame #12: 0x00000001053bbcc5 mbgl.node`void mbgl::mutate<std::__1::vector<mbgl::Immutable<mbgl::style::Source::Impl>, std::__1::allocator<mbgl::Immutable<mbgl::style::Source::Impl> > >, mbgl::style::Collection<mbgl::style::Source>::update(mbgl::style::Source const&)::'lambda'(auto&)>(immutable=0x0000000102a58ac8, fn=<unavailable>)::'lambda'(auto&)&&) at immutable.hpp:125:5 [opt]
    frame #13: 0x00000001053ad8fd mbgl.node`mbgl::style::Collection<mbgl::style::Source>::update(this=<unavailable>, wrapper=<unavailable>) at collection.hpp:135:5 [opt]
    frame #14: 0x00000001053ad8b9 mbgl.node`mbgl::style::Style::Impl::onSourceLoaded(this=0x0000000102a58a00, source=0x0000000102a6cdb0) at style_impl.cpp:272:13 [opt]
    frame #15: 0x00000001053ad91d mbgl.node`non-virtual thunk to mbgl::style::Style::Impl::onSourceLoaded(this=<unavailable>, source=<unavailable>) at style_impl.cpp:0 [opt]
    frame #16: 0x00000001053aaffb mbgl.node`mbgl::style::VectorSource::loadDescription(this=<unavailable>, res=<unavailable>)::$_0::operator()(mbgl::Response) const at vector_source.cpp:70:23 [opt]
    frame #17: 0x00000001053aae08 mbgl.node`decltype(__f=<unavailable>, __args=<unavailable>)::$_0&>(fp)(std::__1::forward<mbgl::Response>(fp0))) std::__1::__invoke<mbgl::style::VectorSource::loadDescription(mbgl::FileSource&)::$_0&, mbgl::Response>(mbgl::style::VectorSource::loadDescription(mbgl::FileSource&)::$_0&, mbgl::Response&&) at type_traits:4353:1 [opt]
    frame #18: 0x00000001053aadb5 mbgl.node`void std::__1::__invoke_void_return_wrapper<void>::__call<mbgl::style::VectorSource::loadDescription(__args=<unavailable>, __args=<unavailable>)::$_0&, mbgl::Response>(mbgl::style::VectorSource::loadDescription(mbgl::FileSource&)::$_0&, mbgl::Response&&) at __functional_base:349:9 [opt]
    frame #19: 0x00000001053aad85 mbgl.node`std::__1::__function::__alloc_func<mbgl::style::VectorSource::loadDescription(mbgl::FileSource&)::$_0, std::__1::allocator<mbgl::style::VectorSource::loadDescription(mbgl::FileSource&)::$_0>, void (mbgl::Response)>::operator(this=<unavailable>, __arg=<unavailable>)(mbgl::Response&&) at functional:1527:16 [opt]
    frame #20: 0x00000001053aa470 mbgl.node`std::__1::__function::__func<mbgl::style::VectorSource::loadDescription(mbgl::FileSource&)::$_0, std::__1::allocator<mbgl::style::VectorSource::loadDescription(mbgl::FileSource&)::$_0>, void (mbgl::Response)>::operator(this=<unavailable>, __arg=<unavailable>)(mbgl::Response&&) at functional:1651:12 [opt]
    frame #21: 0x00000001054a9b23 mbgl.node`std::__1::__function::__value_func<void (mbgl::Response)>::operator(this=<unavailable>, __args=<unavailable>)(mbgl::Response&&) const at functional:1799:16 [opt]
    frame #22: 0x00000001054a8f6c mbgl.node`std::__1::function<void (mbgl::Response)>::operator(this=<unavailable>, __arg=<unavailable>)(mbgl::Response) const at functional:2347:12 [opt]
    frame #23: 0x00000001054a8bf6 mbgl.node`node_mbgl::NodeFileSource::request(this=<unavailable>, resource=0x0000700011f1f8d8, callback=Callback @ 0x0000700011f1f9a0)>) at node_file_source.hpp:39:9 [opt]
    frame #24: 0x00000001053a8ec2 mbgl.node`mbgl::style::VectorSource::loadDescription(this=<unavailable>, fileSource=0x0000000102958e48) at vector_source.cpp:49:22 [opt]
    frame #25: 0x00000001053acc88 mbgl.node`mbgl::style::Style::Impl::addSource(this=0x0000000102a58a00, source=unique_ptr<mbgl::style::Source, std::__1::default_delete<mbgl::style::Source> > @ 0x0000700011f1fc10) at style_impl.cpp:143:13 [opt]
    frame #26: 0x00000001053ac7a6 mbgl.node`mbgl::style::Style::Impl::parse(this=0x0000000102a58a00, json_=<unavailable>) at style_impl.cpp:97:9 [opt]
    frame #27: 0x00000001053ac596 mbgl.node`mbgl::style::Style::Impl::loadJSON(this=0x0000000102a58a00, json_=<unavailable>) at style_impl.cpp:46:5 [opt]
    frame #28: 0x00000001053ab7e9 mbgl.node`mbgl::style::Style::loadJSON(this=<unavailable>, json=<unavailable>) at style.cpp:18:11 [opt]
    frame #29: 0x00000001054a66e8 mbgl.node`node_mbgl::RenderWorker::Execute(this=0x0000000102c1b840) at node_map.cpp:63:24 [opt]
    frame #30: 0x00000001054a6d57 mbgl.node`Napi::AsyncWorker::OnExecute((null)=<unavailable>, this_pointer=0x0000000102c1b840) at napi-inl.h:3723:11 [opt]
    frame #31: 0x00000001009b29fa node`worker + 196
    frame #32: 0x00007fff7f0562eb libsystem_pthread.dylib`_pthread_body + 126
    frame #33: 0x00007fff7f059249 libsystem_pthread.dylib`_pthread_start + 66
    frame #34: 0x00007fff7f05540d libsystem_pthread.dylib`thread_start + 13

This seems to workaround the crash on my system (9c2defe) but I'm unclear if it is the right fix. Would love any feedback anyone has.

[springmeyer]

I've now reduced a testcase for this problem that can be run as part of the gl-native unit tests: #15531.

Can someone from @mapbox/gl-native take a closer look at this?

This problem is now blocking my downstream work because all the workarounds I've been able to come up with all turn out to have unwanted repercussions:

• 9c2defe avoids the invalid array access but leads to the callback to renderStill never being called (essentially hanging my program) when a mbgl::Resource::Source fails to load. It works however when a ``mbgl::Resource::Source` does load without errors.
• d4b4e34 avoids the invalid array access and avoids the hang when a mbgl::Resource::Source fails to load. But it results in 7 failed unit tests in mapbox-gl-native: https://circleci.com/gh/mapbox/mapbox-gl-native/325986?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link. So it seems like it is breaking other core behavior.

[springmeyer]

Solved by #15548, thank you @kkaefer