Skip to content
This repository has been archived by the owner on Sep 30, 2022. It is now read-only.

Fix parsing of envvars in MCA files #973

Merged
merged 3 commits into from
Mar 9, 2016
Merged

Conversation

hjelmn
Copy link
Member

@hjelmn hjelmn commented Feb 18, 2016

This commit fixes a memory corruption bug when parsing lines of the
form:

-x FOO=bar

The code was making changes to the size of the buffer allocated for
key_buffer without making the appropriate changes to
key_buffer_len. This was causing subsequent calls to save_param_name
to write to invalid memory.

This commit makes the following changes:

  • Fix the above bug by modifying trim_name to move the string within
    the buffer instead of re-allocating space for the trimmed string.
  • Cleaned up both trim_name and save_param_name. Both functions took
    a prefix and suffix to trim. Problem was the prefix was not
    treated like a prefix. Instead the "prefix" was located inside the
    string using strstr then the trimmed value started after the
    substring (even in the middle of the string). To allow trimming
    both -x and --x (as well as -mca and --mca) trim_name is now
    called with each prefix.

Signed-off-by: Nathan Hjelm hjelmn@lanl.gov

(cherry picked from commit open-mpi/ompi@3223673)

Fixes open-mpi/ompi#1375

Signed-off-by: Nathan Hjelm hjelmn@me.com

This commit fixes a memory corruption bug when parsing lines of the
form:

-x FOO=bar

The code was making changes to the size of the buffer allocated for
key_buffer without making the appropriate changes to
key_buffer_len. This was causing subsequent calls to save_param_name
to write to invalid memory.

This commit makes the following changes:

  - Fix the above bug by modifying trim_name to move the string within
    the buffer instead of re-allocating space for the trimmed string.

  - Cleaned up both trim_name and save_param_name. Both functions took
    a prefix and suffix to trim. Problem was the prefix was not
    treated like a prefix. Instead the "prefix" was located inside the
    string using strstr then the trimmed value started after the
    substring (even in the middle of the string). To allow trimming
    both -x and --x (as well as -mca and --mca) trim_name is now
    called with each prefix.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>

(cherry picked from commit open-mpi/ompi@3223673)

Fixes open-mpi/ompi#1375

Signed-off-by: Nathan Hjelm <hjelmn@me.com>
@hjelmn
Copy link
Member Author

hjelmn commented Feb 18, 2016

:bot🏷️bug
:bot:milestone:v2.0.0
:bot:assign: @jsquyres

@hppritcha If this is too much for v2.0.0 please go ahead and bump to v2.0.1. This is a new feature in v2.x and it is currently broken.

@ompiteam-bot ompiteam-bot added this to the v2.0.0 milestone Feb 18, 2016
@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1349/ for details.

@hjelmn
Copy link
Member Author

hjelmn commented Feb 18, 2016

Should probably mention that all Mellanox Jenkins tests should be failing without this. I ran their Jenkins script on a TLCC2 system and the -tune tests abort due to memory corruption. Valgrind identified the exact source of the error.

/* trim spaces at the beginning */
while (' ' == *pchr || '\t' == *pchr) {
while (isspace (*pchr)) {
Copy link
Member

Choose a reason for hiding this comment

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

I realize that this is the way it was before you fixed it -- but shouldn't the prefix check come after the beginning-of-string white space trim?

I ask because the suffix check comes after the end-of-string white space trim.

@hppritcha
Copy link
Member

works for me now with srun/alps/mpirun 👍

@hppritcha
Copy link
Member

sorry wrong PR.

@hppritcha hppritcha removed the reviewed label Mar 1, 2016
@hppritcha
Copy link
Member

removed the reviewed label since that was accidentally generated by my erroneous comment.

@hppritcha
Copy link
Member

@hjelmn could you incorporate @jsquyres 's feedback so we can merge?

@hjelmn
Copy link
Member Author

hjelmn commented Mar 7, 2016

sure.

This commit fixes a bug in the opal key value parser that might cause
the filename parser to go past the beginning of the string.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>

(cherry picked from open-mpi/ompi@63bac9a)

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1406/ for details.

@jsquyres
Copy link
Member

jsquyres commented Mar 8, 2016

@hjelmn Squash into one commit, and we're good.

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>

(cherry picked from open-mpi/ompi@607be72)

Signed-off-by: Nathan Hjelm <hjelmn@lanl.gov>
@jsquyres
Copy link
Member

jsquyres commented Mar 8, 2016

Oh, these are master commits -- so we can't squash. Ok.

@jsquyres
Copy link
Member

jsquyres commented Mar 8, 2016

👍

@jsquyres
Copy link
Member

jsquyres commented Mar 8, 2016

@hppritcha Good to go

@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1412/ for details.

hppritcha added a commit that referenced this pull request Mar 9, 2016
Fix parsing of envvars in MCA files
@hppritcha hppritcha merged commit 98ac7a8 into open-mpi:v2.x Mar 9, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MCA parameter file parsing of env vars is broken
5 participants