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

protect the info passed to pmix_tool_connected_fn #1871

Closed
wants to merge 1 commit into from

Conversation

HawkmoonEternal
Copy link

The info array passed to pmix_tool_connected_fn is also part of the cbdata (i.e. a member of the pmix_pending_connection_t struct) and gets assigned to the pmix_server_req_t struct.

When calling the callback function provided by the openpmix server implementation, the PMIx server releases the pmix_pending_connection_t struct (including the info array).
Likewise, PRRTE releases the pmix_server_req_t struct (including the info array).
Thus a double free happens on the info array, potentially leading to invalid memory accesses.

This patch protects the info passed to pmix_tool_connected_fn by assigning a copy of the info to the pmix_server_req_t struct instead of the original info.

@rhc54
Copy link
Contributor

rhc54 commented Nov 30, 2023

We probably shouldn't be releasing those info structs in PRRTE - probably should have a "const" in front of them in the upcall signature (I'll propose that over in the standard). I'd suggest instead adding:

cd->info = NULL;
cd->ninfo = 0;

right before the PMIX_RELEASE(cd) call in line 658 to protect those structs.

@rhc54
Copy link
Contributor

rhc54 commented Dec 1, 2023

Actually, I'm confused by this report. I looked at the PRRTE pmix_server_req_t destructor and it does not release the info structs pointed to by that object. So it looks to me like this patch actually creates a memory leak since the copied infos are never released.

Can you point me to where the double-free is occurring in PRRTE?

@HawkmoonEternal
Copy link
Author

Oof, apologies for my report.
It seems that this issue resulted from me mixing up a local version with some other version I was preparing for integrating PMIX_ALLOC_ID into PMIx_Spawn. Somehow things got messed up there.

I'll close this pull request - sorry for taking your time.

I will add a pull request for the PMIX_ALLOC_ID feature in the coming days.

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.

2 participants