[Fix] Provided MPI_type_get_content array sizes may exceed actual ones, do not use them #13131
Conversation
…s, do not use them MPI 4.1 specifications, page 166, lines 24-27: The values given for max_integers, max_addresses, max_large_counts, and max_datatypes must be at least as large as the value returned in num_integers, num_addresses, num_large_counts, and num_datatypes, respectively, in the call MPI_TYPE_GET_ENVELOPE for the same datatype argument. Signed-off-by: Brelle Emmanuel <emmanuel.brelle@eviden.com>
There was a problem hiding this comment.
Why do you need this ? ompi_datatype_get_args has a protection against this case, that's why the memcpy is based on the internal value and not on the value provided by the user.
| } | ||
| } | ||
| | ||
| /* Counts may exceed actual ones, we have no choice but to recompute them */ |
There was a problem hiding this comment.
Not sure I understand that comment: what are the actual ones? The count argument the user provided? Or the
| /* Counts may exceed actual ones, we have no choice but to recompute them */ | |
| /* Counts may exceed the user-provided counts, we have no choice but to recompute them */ |
| rc = ompi_datatype_get_args(mtype, 0, &max_integers, NULL, &max_addresses, NULL, &max_datatypes, | ||
| NULL, &type_code); | ||
| if (rc != MPI_SUCCESS) { | ||
| OMPI_ERRHANDLER_RETURN(MPI_ERR_INTERN, MPI_COMM_WORLD, MPI_ERR_INTERN, FUNC_NAME); | ||
| } | ||
| |
There was a problem hiding this comment.
What is the point of querying the sizes if we don't act on that information? If the user told us the arrays are of a certain size and the type requires more then we should error out, raising MPI_ERR_ARG.
There was a problem hiding this comment.
This updates the user provided max lengths before passing them to ompi_datatype_get_args. As if ompi_datatype_get_args was not correctly handling the case where user provides more space than required (which is not the case as I pointed in my prior comment).
There was a problem hiding this comment.
I'd be worried about the converse: the user provides insufficient space and we write past the bounds of the user buffer. That is what I thought this patch addressed. Otherwise I don't see what this patch accomplishes.
There was a problem hiding this comment.
The patch cannot accomplish that, it never checks if the arrays' sizes are legitimate but instead it simply tells the OMPI datatype engine that everything is OK disregarding the user provided sizes for the arrays.
We need that because of this line ompi/ompi/mpi/c/type_get_contents.c Line 76 in 8a64826 |
| Make sense, that code needs to know the correct number of datatypes in order to work properly. As we have control over Also, the code you pointed at is incorrect, in case of error it only frees the current datatype leading to memory leaks. Let me take a stab at it, I'll get back to you. |
3 participants
MPI 4.1 specifications, page 166, lines 24-27:
The values given for max_integers, max_addresses, max_large_counts, and max_datatypes must be at least as large as the value returned in num_integers, num_addresses, num_large_counts, and num_datatypes, respectively, in the call MPI_TYPE_GET_ENVELOPE for the same datatype argument.