Introduce MaybeSharedPtr for managing user facing objects

[tmadlener]

BEGINRELEASENOTES

• Introduce the MaybeSharedPtr to manage the Obj* in the user facing handle classes.
  • This splits the control block and the managed object into two distinct entities with potentially different lifetimes, which allows to fix heap-use-after-free in ExampleCluster #174 and AddressSanitizer: heap-use-after-free in object destructor #492.
  • This increases the size of the user facing handle classes by a factor two, since they are now effectively two pointers instead of one, even if the control block will not be initialized in case a handle is obtained from a collection.
• Remove the ObjBase base class and make the ObjectID a member of the Obj classes.
• Make the user facing handle class constructors from an Obj* private as users will not have access to raw Obj* in any case.
  • Introduce a static makeEmpty method for the generated classes in order to create an empty handle, which is also used internally to handle unpersisted relations.
• Enable more existing test cases in sanitizer workflows now that it has become possible to do so.

ENDRELEASENOTES

This is a resurrected version of #220 with some minor modifications. I will summarize things again here, so that things are in one place and easier to follow. Similar to that one this should address the heap-use-after-free issues

• Fixes heap-use-after-free in ExampleCluster #174
• Fixes AddressSanitizer: heap-use-after-free in object destructor #492
• Fixes Mutability conversion fails on push_back to collection via Python bindings #431
• Supersedes [WIP] Improve user facing API: make Object ctor from Obj* private #221 (see below for why)

The main thing this PR introduces is the MaybeSharedPtr, a smart pointer, that can relinquish ownership of the managed object while still being safely destructible even if the originally managed object has been destroyed. It achieves this by having a simple control block that tracks the lifetime of the MaybeSharedPtr and optionally that of the managed object. Ownership of the managed pointer can only move from the MaybeSharedPtr to something else, but never the other way around. It avoids the heap-use-after-free issues by keeping the control block alive until all references to it are destroyed. Previously this control block lived inside the managed object, so that the potentially separate lifetimes caused issues.

In order to keep backwards compatibility it is used for all user facing objects. However, the internal control block will only be instantiated and initialized if the user facing object is created "freestanding", i.e. not obtained from a collection via create or via one of the accessor methods. Even though the control block is unused in these cases, it still occupies space and all user facing handles have now double the previous size, i.e. 2 pointers. This should still be small enough to allow for simple pass-by-value function calls efficiently. Overall the behavior is the same as previous but the heap-use-after-free issues should be gone, since the control block will always live long enough to be safely destructed. The documentation has been updated to clearly state the expectations on the validity of handles that have been added to a collection. This is purely a documentation change, nothing in the behavior has change in this respect.

As a small benefit the public constructors starting from an Obj* have been removed, since they are not usable in any case as there is no (easy) way in which users could obtain such a pointer. Instead a new makeEmpty function is introduced to make it possible to create "empty handles" for handling unpersisted relations (without having to expose a public constructor from an Obj*). These object will always return false to a call to isAvailable().

As a smaller detail this also removes the podio::ObjBase and moves the ObjectID into the Obj as an additional member.

Using makeEmpty to detect uninitialized handles

In some cases it makes sense to declare a default constructed immutable object (handle) and then assign to that in the course of an algorithm, e.g.:

MCParticle particle;
if (condition) { 
  particle = /* result of e.g. some lookup */
}

In case condition is never true, particle will contain a default initialized handle (i.e. most likely 0s only), which may be entirely useless. However, without explicitly checking whether it is default initialized it is impossible to tell whether this is in a useful state or not. It is possible to use the makeEmpty function to initialize this handle to something that will crash if used without reassigning via:

auto particle = MCParticle::makeEmpty();
if (condition) {
  particle = /* result of e.g. some lookup */
}
if (particle.isAvailable()) {
  // particle has been assigned a proper value
}

Now, if condition is never true, accessing particle will crash at runtime. However, it is also possible to check whether an assignment has happened by calling isAvailable() first.

• Release notes
• Documentation

[tmadlener]

@veprbl does this look like something that could work for EIC / Jana2? I have added the minimal reprodcuer from the original issue as a test-case that previously triggered under builds with AddressSanitizer enabled, but now no longer does.

Do the list of potential issues so far look like something that would make it hard / impossible for you to adapt this?

[tmadlener]

This will break quite a few things in CEPCSW, mostly around maps or some lookup algorithms. In at least a few cases it's not entirely clear whether things will still work if objects are initialized as empty, or whether something downstream expects at least a usable object. Potentially we should consider introducing a makeDefault factory method as well that returns a (default) initialized object, something like

static makeDefault() {
  const static auto defaultObj = Obj{};
  return {defaultObj};
}

[veprbl]

I've tried adapting our code to this branch, and the resulting diff is quite small, however, the makeEmpty() doesn't allow it to be forward compatible. And, I found that even though std::map::operator[], like you say, doesn't compile, the at() method still works. I do get a random crash in runtime:

       `- JSignalHandler::handle_sigsegv(int, siginfo_t*, void*) (0x7ffff7f6b7a1)
        `- libc.so.6 (0x7ffff563dbf0)
         `- edm4eic::Cluster::getHits(unsigned long) const (0x7ffff4f144cf)
          `- eicrecon::ImagingClusterReco::fit_track(std::vector<edm4eic::Cluster, std::allocator<edm4eic::Cluster> > const&) const (0x7ffff3223571)

Haven't debugged it, but our access pattern looks quite reasonable:
https://github.com/eic/EICrecon/blob/fdc79ac42bf27b9000e2a9c752b5b9cfc6a85ba4/src/algorithms/calorimetry/ImagingClusterReco.h#L271

[tmadlener]

Thanks for checking this.

It looks a bit like your crash is because your Cluster comes from a makeEmpty and hasn't been assigned a "proper" cluster yet. I am mainly guessing this from the stacktrace and the fact, that Cluster::getHits(unsigned). It is generated from these bits here:

podio/python/templates/macros/implementations.jinja2

Lines 135 to 140 in 858c0ff

	{{ relation.full_type }} {{ class_type }}::{{ relation.getter_name(get_syntax) }}(std::size_t index) const {
	if ({{ relation.name }}_size() > index) {
	return m_obj->m_{{ relation.name }}->at(m_obj->data.{{ relation.name }}_begin + index);
	}
	throw std::out_of_range("index out of bounds for existing references");
	}

Effectively the implementation will try to access the internal m_obj which is a nullptr if initialized via makeEmpty and will hence crash.

I am not sure what is the best solution in this case here. We could also bring back the default constructor. That would fix this, and other, crashes. On the other hand it might hide subtle issues where people access objects that are default initialized, expecting something more useful.

I think we should schedule a brief meeting to discuss which approach works best for you in this case. That might be quicker and easier to do than over github issues.

[tmadlener]

I have now also made the default handles ref-counted / managed via a MaybeSharedPtr (as also suggested in #492). This doubles their size to two pointers. However, if they are in any form obtained from a collection the control block will not be initialized (it will remain nullptr). This means that almost all checks for the control block will be a simple check against nullptr. The main reasons for doing this are:

• Keep the existing behavior (but remove the use-after-free)
• Make dangling references a bit harder to create

This will now work as expected (this has become a test case):

std::vector<ExampleHit> hits;
{
  auto hit = MutableExampleHit{};
  hits.push_back(hit);
}
auto e = hits[0].energy();

The only thing that is still creating dangling references is something along the lines of

auto hit = ExampleHit::makeEmpty();
{
  auto hits = ExampleHitCollection();
  auto mHit = hits.create();
  hit = mHit;
}
auto e = hit.energy(); // <--- heap-use-after-free

However, since this is pre-existing behavior (although not documented well enough probably), I would argue that this is OK.

[tmadlener]

I also brought back the pre-existing constructors for the default, immutable handles. With this in place this PR should now be completely transparent in fixing the reported use-after-free issues in destructors. Any other use-after-free from using "dangling references" are still there. These are not easily fixable in the current model, but fixing that requires some really breaking changes. I have, however, tried to at least document that these dangling references / invalid handles can happen if a handle outlives its collection.

[tmadlener]

I did some brief benchmarking using the k4EDM4hep2LcioConv converter as that seemed like an easy and reasonable choice as it handles creation of new data as well as reading them back (and touching all members / relations) in the tests. I did not measure any real difference there. So I think from that point of view this approach is fine.

@veprbl this should now be fully backward compatible if you don't have handle constructors from Obj* (switching to makeEmpty would be the solution there). Apologies for the intermediate confusion with the removed handle constructors and more breakage. If you have any easy to run benchmarks / profiling, I would also be interested in whether you see the same (unchanged) behavior as I did.

[tmadlener]

I also just realized that this fixes #431 because it introduces a dedicated overload for Collection::push_back for Mutable types. Hence, there is no longer a need for an implicit conversion on the python side.

[veprbl]

Hi @tmadlener. I've run some reconstruction benchmarks with EICrecon, and I see no difference in timing with PODIO from this branch vs the release version.

[tmadlener]

Thanks a lot for testing. Also good that you see similar behavior than me. In that case I would vote for merging this and revisit this in case performance (ever) becomes an issue around this.