-
Notifications
You must be signed in to change notification settings - Fork 281
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
hydra: remove hard-coded token limit #6682
Conversation
0f68836
to
09c7855
Compare
test:mpich/ch3/tcp ✔️ |
test:mpich/ch4/ofi A memory leak in the spawn path, detected by the |
The hard limit of HYD_NUM_TMP_STRINGS is potential for unsafe code. The code here is safe, but make it local and not depend on arbitrary macro is cleaner.
When parsing config file, it first load tokens into a pre-allocated array of size of HYD_NUM_TMP_STRINGS (1000). The hard limit is not safe in case of very large number of arguments. Use utarray instead. It seems the previous code also leaks the strdup'ed token strings. This commit fixes that as well.
Avoid arbitrary limit (HYD_NUM_TMP_STRINGS) on number of hosts, use utarray instead.
Avoid hard limit of HYD_NUM_TMP_STRINGS, use dynamic array instead. Also allocate exact size for each token string rather than HYD_TMP_STRLEN (16K) for each.
It is unsafe to use arbitrary limit -- HYD_NUM_TMP_STRINGS -- to static allocate exec arg list. Use dynamic array instead.
09c7855
to
0da1732
Compare
test:mpich/ch4/ofi ✔️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I originally filed this issue. May I know in which version the fix will be available? Is there an ETA? |
It will be in release 4.2. We will release mpich-4.2b1 early this November, and GA release in December or early next January. |
We are using 3.2.1 version. Is there a way or feasible even to merge the change in 3.2.1. Moving to 4.2 at this time is not feasible for us. |
You could try install/upgrade hydra separately. There is a separate hydra tarball in every release. |
Is it possible to cherry-pick these changes in 3.2.1? |
I think separately upgrade hydra is more feasible. 3.2.1 is very old at this point. Hydra has gone through significant refactor since then. |
Pull Request Description
There are multiple places in hydra that use a hard-coded macro
HYD_NUM_TMP_STRING
(set to 1000) for temporary arrays. This places unnecessary limit as very long file lines or command lines, while not usual, do appear in real usages. In addition, the hard limit embeds pitfalls for code to segfault if boundary checking is missing. Using dynamic arrays fixes these issue.Fixes #6681
[skip warnings]
Notes
The remaining usage of
HYD_NUM_TMP_STRINGS
are used for constructinghydra_pmi_proxy
command line argument. Proxy argument list is controlled and should be well below 1000. Majority of argument and environment list are passed in via socket connection. It may still worth for some cleanup. This is left for later.Author Checklist
Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
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.
Whitespace checker. Warnings test. Additional tests via comments.
For non-Argonne authors, check contribution agreement.
If necessary, request an explicit comment from your companies PR approval manager.