Thread safety issue in `TypeSequenceManager::find`

[Patrick Shriwise]

I recently discovered an issue in the TypeSequenceManager::find method that has to do with the lastReferenced data member.

https://bitbucket.org/fathomteam/moab/src/f2ca19ef1d45a9a08316ba508501c142e9ce4d3f/src/TypeSequenceManager.hpp#lines-387:414

One place that's an issue is between the check for the EntityHandle in the last referenced sequence and setting the sequence.

 else if( h >= lastReferenced->start_handle() && h <= lastReferenced->end_handle() )
    {
        seq = lastReferenced;
        return MB_SUCCESS;
    }

It's possible that between the if statement and setting seq, the lastReferenced variable will be updated by another thread, causing an invalid sequence to be returned.

Something similar happens here where lastReferenced is set and seq is set to lastReference.

    seq = lastReferenced = *i;

A minimal set of changes to avoid this problem is here:

https://bitbucket.org/pshriwise/moab/commits/e4710a755e06e5d309f9af4819fc2742ec77d414

And I think I have a workable fix for this method to avoid these problems here:

https://bitbucket.org/pshriwise/moab/commits/6f1b42e710fed53c00128625c54a12a57fafa350

This gets exposed by the DAGMC overlap_check_test

[Vijay M]

Thanks @pshriwise. I have a branch somewhere, probably local on my laptop that has a fix I implemented after our last discussion regarding this issue. I'll take a look at your changes, but feel free to open a PR for review as well with a test example to replicate problem and a fix.

[Patrick Shriwise]

I’ll open a PR shortly!

Do you have a recommendation on how to write the test? It isn’t clear to me how to do that. Since this is a race condition we can’t guarantee that we’ll be able to reproduce the issue for any given run of the test.

[Vijay M]

@Patrick Shriwise Can I close this issue ? The PR to address the fixes has been merged and is now part of 5.3.0 already. Is there anything else to follow up on this ?

[Patrick Shriwise]

Nothing else to follow up on AFAIK. Correct! Closing now.