Skip to content
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.

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

romio: fix MPIU_external32_buffer_setup #5907

Merged
merged 1 commit into from
Apr 7, 2022
Merged

Conversation

hzhou
Copy link
Contributor

@hzhou hzhou commented Mar 25, 2022

Pull Request Description

It should allocate a buffer size of true_lb + true_extent rather than
just true_extent.

Fixes #5885

[skip warnings]

Author Checklist

  • Provide Description
    Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
  • Commits Follow Good Practice
    Commits are self-contained and do not do two things at once.
    Commit message is of the form: module: short description
    Commit message explains what's in the commit.
  • Passes All Tests
    Whitespace checker. Warnings test. Additional tests via comments.
  • Contribution Agreement
    For non-Argonne authors, check contribution agreement.
    If necessary, request an explicit comment from your companies PR approval manager.

@ashwinraghu
Copy link

This looks promising. I've run some of the configurations of the benchmark that used to consistently fail and they all have passed with this change. A valgrind check also has come out clean w.r.t invalid writes.
I will run some multi-node tests.

@ashwinraghu
Copy link

Multi-node tests look good as well.

@ashwinraghu
Copy link

I am a little puzzled as to why 4.0.1 doesn't seem to hit this issue because MPIU_datatype_full_size() looks identical there too.

@hzhou
Copy link
Contributor Author

hzhou commented Apr 6, 2022

I am a little puzzled as to why 4.0.1 doesn't seem to hit this issue because MPIU_datatype_full_size() looks identical there too.

I just checked with current main branch, the same invalid writes show up in valgrind without this patch. Segfault sometimes does not trigger due to random runtime memory usage.

@hzhou hzhou requested a review from raffenet April 6, 2022 16:46
@hzhou
Copy link
Contributor Author

hzhou commented Apr 6, 2022

test:mpich/ch3/most
test:mpich/ch4/most
✔️

Copy link

@ashwinraghu ashwinraghu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

It should allocate a buffer size of true_lb + true_extent rather than
just true_extent.
@hzhou hzhou force-pushed the 2203_romio_ext32 branch from e566381 to 2e7a0bb Compare April 7, 2022 13:54
@hzhou hzhou merged commit b2b1423 into pmodels:main Apr 7, 2022
@hzhou hzhou deleted the 2203_romio_ext32 branch April 7, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buffer overflow in MPI-IO with external32 data representation
3 participants