- changed status to open
Thread safety issue in `TypeSequenceManager::find`
I recently discovered an issue in the TypeSequenceManager::find
method that has to do with the lastReferenced
data member.
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
Comments (5)
-
-
reporter 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.
-
@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 ?
-
reporter Nothing else to follow up on AFAIK. Correct! Closing now.
-
reporter - changed status to resolved
- Log in to comment
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.