Invalid iterator comparison on shutdown

[andrewkcorcoran]

With g++ debug checking enabled (-D_GLIBCXX_SANITIZE_VECTOR -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC") the following error is encountered on shutdown.

/opt/rh/devtoolset-8/root/usr/lib/gcc/x86_64-redhat-linux/8/../../../../include/c++/8/debug/safe_iterator.h:552:
    error:
    attempt
    to
    compare
    a
    singular

    iterator
    to
    a
    past-the-end

    iterator
    .

Objects involved in the operation:
iterator "lhs" @ 0x0x287e2e8 {
type = ▒hI1▒ (mutable iterator);
  state = singular;
  references sequence with type `NSt7__debug6vectorISt10shared_ptrIN6binlog7Session7ChannelEESaIS5_EEE' @ 0x0x287e2e8
}
iterator "rhs" @ 0x0x287e2e8 {
type = ▒hI1▒ (mutable iterator);
  state = past-the-end;
  references sequence with type `NSt7__debug6vectorISt10shared_ptrIN6binlog7Session7ChannelEESaIS5_EEE' @ 0x0x287e2e8
}

[erenon]

Thanks for reporting this, very interesting. Given that the Session class doesn't have a user defined destructor, and doesn't use lhs, rhs variables either, isn't this a bug in the destructor of vector itself? Perhaps this is reproducible with a simple, binlog-less example?

(writing this from a phone, my reading of the code might be incorrect)

[andrewkcorcoran]

Is it possible it's a race condition between the user thread calling binlog::consume and the destruction of the Session? Since Session::consume is iterating over std::vector<std::shared_ptr<Channel>> _channels.

[erenon]

Possible, but in that case, that is an Application error, right? binlog::consume should not be called, while the library is being unloaded (causing the destruction of the function local static session)

[andrewkcorcoran]

The issue is with [removing the last element of `_channels`]( https://github.com/Morgan-Stanley/binlog/blob/master/include/binlog/Session.hpp#L379 ). [`std::vector::pop_back`]( https://en.cppreference.com/w/cpp/container/vector/pop_back) invalidates both the `end()` iterator and iterators and references to the last element. Therefore when removing the last element, calling `pop_back` invalidates the `it` iterator (which is pointing at the last element) and then when the for loop conditional is re-evaluated (`it != _channels.end()`) `it` is invalid.

…

On Tue, 18 Aug 2020, 12:46 pm Thaler Benedek, ***@***.***> wrote: Possible, but in that case, that is an Application error, right? binlog::consume should not be called, while the library is being unloaded (causing the destruction of the function local static session) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#88 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AL5LS3GQCMXRPC3TUW6IO3TSBI5UZANCNFSM4QBTQ2CQ> .

[erenon]

Good catch, this needs to be fixed.