Skip to content

Fix integer overflows with indexed datatype creation. #6482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 13, 2019
Merged

Fix integer overflows with indexed datatype creation. #6482

merged 1 commit into from
Mar 13, 2019

Conversation

@awlauria
Copy link
Contributor

@awlauria awlauria commented Mar 13, 2019 •
edited
Loading

The types of count, disp, and extent passed into
ompi_datatype_add() should be size_t, ptrdiff_t and ptrdiff_t,
respectively. This prevents integer overflows and errors in
computing the size of large indexed datatypes.

Signed-off-by: Austen Lauria [email protected]

@ompiteam-bot
Copy link

Can one of the admins verify this patch?

@jsquyres
Copy link
Member

ok to test

@jsquyres
Copy link
Member

Looks like a false Mellanox Jenkins failure

bot:mellanox:retest

@jsquyres jsquyres requested a review from bosilca March 13, 2019 13:37
@awlauria
Fix integer overflows with indexed datatype creation.
b61e624 
The types of count, disp, and extent passed into
ompi_datatype_add() should be size_t, ptrdiff_t and ptrdiff_t,
respectively. This prevents integer overflows and errors in
computing the size of large indexed datatypes.

Signed-off-by: Austen Lauria <[email protected]>
@awlauria awlauria force-pushed the indexed_datatype_overflows branch from 831a328 to b61e624 Compare March 13, 2019 13:40
@jjhursey
Copy link
Member

I'm not sure why the IBM CI didn't fire on this PR. Let's try to force it.
bot:ibm:retest

@bosilca bosilca merged commit 0173796 into open-mpi:master Mar 13, 2019
@jsquyres
Copy link
Member

I assume this is a somewhat esoteric bug fix (i.e., not likely to happen in many real world cases).

  • Should we cherry pick it to v4.0.x?
  • Should we cherry pick it to v3.1.x / v3.0.x?

@jjhursey
Copy link
Member

It's a bit of an edge case, but a solid fix. We will want this in at least the v4.0.x series, and should be considered for the v3.x series too - IMHO.

@awlauria
Copy link
Contributor Author

awlauria commented Mar 13, 2019 •
edited
Loading

For some more context on how it was found, it can be reproduced by calling MPI_Type_create_hindexed(2238, 477312832, 477321792, MPI_BYTE, &type).
followed by querying the size MPI_Type_size_x(type, &size) would show a negative value returned in size.

So it is probably not a common case.

@jsquyres
Copy link
Member

That works for me. I 👍'ed @jjhursey's post; I wouldn't mind seeing this PR'ed to v3.0.x/v3.1.x/v4.0.x.

@awlauria
Copy link
Contributor Author

Sure.

4.0.x: #6485
3.1.x: #6483
3.0.x: #6484

@awlauria awlauria deleted the indexed_datatype_overflows branch March 13, 2019 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bosilca bosilca bosilca approved these changes

Assignees

No one assigned

Labels

bug code cleanup Target: main

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@awlauria @ompiteam-bot @jsquyres @jjhursey @bosilca