Issues running OMPI with address sanitizer

[nivShpak]

mpicc and ompirun fail to run when compiled with "-fsanitize=address" due to a memory overflow in OPAL arg parsing.

[devreal]

@nivShpak Could you please provide the error you encounter? It's not clear what #11827 really fixes.

[nivShpak]

in function opal_argv_count()

int opal_argv_count(char **argv)
{
    char **p;
    int i;

    if (NULL == argv) {
        return 0;
    }

    for (i = 0, p = argv; *p; i++, p++) {  //<=
        continue;
    }

    return i;
}

it accesses the array until it gets to its end. This causes a heap overflow that prohibits address sanitizer from running.
When I try to compile an ompi application using mpicc the mpicc is crushing at that point.
the fix avoids this overflow access so we can debug with -fsanitize=address flag.

[jsquyres]

@nivShpak I agree with @devreal -- I don't see how the code snipit you showed above (opal_argv_count()) is related to the enum that you added in #11827. Can you explain in more detail? Thanks.

[nivShpak]

When it counts the args (*p) after the third it accesses a memory out on the range.
This access fails the program if it was compiled with address sanitizer
In order to run an OMPI app with address sanitizer, we need to compile ompi with it, but mpicc and prirun cannot run with address sanitizer because of this.

[devreal]

@nivShpak It's still not clear to me. Can you please a) post the output of address sanitizer; and b) explain how the added enum member changes the behavior of the loop you posted?

[artpol84]

@jsquyres @devreal
we encountered the issue when trying to use Address Sanitizer as @nivShpak mentioned.

The problem is the following. Currently, the size of the array is exactly the number of colors.

ompi/opal/runtime/opal_params_core.h

Line 50 in 1717930

extern char *opal_var_dump_color[OPAL_VAR_DUMP_COLOR_KEY_COUNT];

However, the code snippet provided by @nivShpak

  for (i = 0, p = argv; *p; i++, p++) {  //<=

Assumes that there is a NULL element in the end.
The idea of the fix is to introduce one extra element at the end of the array to address that.

[jsquyres]

Are you saying that opal_var_dump_color[OPAL_VAR_DUMP_COLOR_KEY_COUNT - 1] != NULL? If so, isn't that what should be fixed?

It feels weird to me to add a meaningless enum that has a side effect of lengthening an array that may have a further side effect of fixing a memory error. I think the real questions are:

• Is opal_var_dump_color[] not long enough?
  • If it's not long enough, add 1 to the length -- don't add a meaningless enum.
• Is opal_var_dump_color[] not initialized properly?
  • If it's not initialized properly, then initialize it properly.

Or am I missing something?

[artpol84]

Are you saying that opal_var_dump_color[OPAL_VAR_DUMP_COLOR_KEY_COUNT - 1] != NULL?

Correct

It feels weird to me to add a meaningless enum that has a side effect of lengthening an array that may have a further side effect of fixing a memory error. I think the real questions are:

• Is opal_var_dump_color[] not long enough?

Yes it's not.
We chose to use ENUM because the enum element is currently used to identify its length:

typedef enum {
...
    OPAL_VAR_DUMP_COLOR_KEY_COUNT
} opal_var_dump_color_key_t;
extern char *opal_var_dump_color[OPAL_VAR_DUMP_COLOR_KEY_COUNT];

Maybe using a different name would be better, like OPAL_VAR_DUMP_COLOR_VALID_NULL_TERM would make it cleaner as it will explain that there's actually an element in the array that has to be there to ensure correctness.
As ENUM is used for indexing it might be a good idea to cover all of the cases explicitly for maintenance purpose

If the naming space is precious and you don't want to pollute it with the NULL_TERM element, the other solution would be to define the macro to calculate the array size;

typedef enum {
    OPAL_VAR_DUMP_COLOR_VAR_NAME = 0,
    OPAL_VAR_DUMP_COLOR_VAR_VALUE = 1,
    OPAL_VAR_DUMP_COLOR_VALID_VALUES = 2,
    OPAL_VAR_DUMP_COLOR_KEY_COUNT
} opal_var_dump_color_key_t;

#define OPAL_VAR_DUMP_COLOR_SIZE (OPAL_VAR_DUMP_COLOR_KEY_COUNT + 1)
extern char *opal_var_dump_color[OPAL_VAR_DUMP_COLOR_SIZE];

• If it's not long enough, add 1 to the length -- don't add a meaningless enum.
• Is opal_var_dump_color[] not initialized properly?

No. As long as it's long enough I believe that the static nature of the array allows it to be initialized correctly.

[artpol84]

@nivShpak also, I assume we should add a more meaningful description to the commit message to detail what is the fix.
The confusion here is confirming that.

[jsquyres]

I think declaring the array like this is fine:

char *opal_var_dump_color[OPAL_VAR_DUMP_COLOR_KEY_COUNT + 1];

It's well known/understood in C that if you have a NULL-terminated array, then you need to have an additional element. I think having an otherwise-meaningless enum would be a little weird, especially since there's no other precedent for that in OMPI (i.e., if the OMPI code base commonly used that convention, that would probably be acceptable -- but as a one-off, it's a little weird).

Alternatively, if using opal_argv_count() is the Wrong Thing, and we should instead be explicitly passing around the length of the array, or know that the length of the array is OPAL_VAR_DUMP_COLOR_KEY_COUNT, that would also be fine. The usage would need to be updated in all the appropriate places.

[artpol84]

I think declaring the array like this is fine:

char *opal_var_dump_color[OPAL_VAR_DUMP_COLOR_KEY_COUNT + 1];

Ok, @nivShpak please change accordingly.

Alternatively, if using opal_argv_count() is the Wrong Thing, and we should instead be explicitly passing around the length of the array, or know that the length of the array is OPAL_VAR_DUMP_COLOR_KEY_COUNT, that would also be fine. The usage would need to be updated in all the appropriate places.

That sounds like a more disruptive task that @nivShpak don't have cycles for. So I'd stick to the short-term fix and have that other item in the TODO list for later.