Prevent a segfault when accessing a rank outside a communicator. #6538
Conversation
This is not fixing any issue, it is simply preventing a sefault if the communicator creation has not happened as expected. Thus, this code path should never really be hit in a correct MPI application with a valid communicator creation support. Signed-off-by: George Bosilca <[email protected]>
| */ | ||
| if( OPAL_UNLIKELY(rank >= (int)pml_comm->num_procs) ) { | ||
| ompi_rte_abort(-1, "PML OB1 received a message from a rank outside the" | ||
| " valid range of the communicator. Please submit a bug request!"); |
There was a problem hiding this comment.
Shouldn't this invoke the error handler on the communicator -- not just always abort?
There was a problem hiding this comment.
As the comment states, if we ever hit this case the communicator creation did something really bad and the communicator is broken globally, so there is only one realistic path forward, abort the job.
There was a problem hiding this comment.
This is a funny statement coming from the ULFM guy. 😉
Shouldn't we let the application try to save its own state (e.g., if they selected ERRORS_RETURN)? I.e., yes, the state of OMPI is borked -- but it still may be desirable to do something outside the scope of MPI.
There was a problem hiding this comment.
My position remains coherent, ULFM is about controlled behaviors in well understood scenarios (mainly process fault) while this particular PR is about coping with a broken state of the MPI implementation itself.
It makes no sense to allow the error handler on the communicator in question to be called, simply because we know (due to the triggered condition) that, at least, this communicator (and potentially all communicators created after it) are completely broken (mismatched cid on the participants). If we escalate the issue to MPI_COMM_WORLD, and trigger the error handler there, it might come at an unexpected time for the application leading to even more badness.
To summarize my position, when we reach this condition there is a possibility that we have already mismatched messages in this broken communicator, which would result in the state of the MPI application being inconsistent and thus unsafe for saving.
There was a problem hiding this comment.
Fair enough points. Should we have an error code about that? E.g., MPI_ERR_INTERNAL_STATE_IS_BOKED? That would let the application decide whether it wants to save or just abort.
There was a problem hiding this comment.
It would clearly be a better approach. However, we currently profusely call ompi_rte_abort in all critical places where we do not want to clean the return path. Fixing this is a desirable long term improvement, until then this patch provides a bandaid (that hopefully will never be triggered).
2 participants
This is not fixing any issue, it is simply preventing a sefault if the
communicator creation has not happened as expected. Thus, this code path
should never really be hit in a correct MPI application with a valid
communicator creation support.
This PR provides a better conclusion to #6522.
Signed-off-by: George Bosilca [email protected]