scr exansion of ENV variables in configuration options leads to memory leaks

The code to expand environment variables in configurations values makes a copy of the env variable value using strdup() but is the only case where scr_param_get() returns a copy so that users cannot call free() on its return value. The relevant code snipped looks like this:

  value = kvtree_elem_get_first_val(scr_system_hash, name);
  if (value != NULL) {
    /* evaluate environment variables */
    if(*value == '$'){
      // RH: this case returns a copy while all others do not
      value = strdup(getenv(value+1));
    }
    return value;
  }

and appears a couple of times in scr_param.c.

A related issue is the code in param_get_hash() around

https://github.com/LLNL/scr/blob/82a3c43d8243c61fac331b64539535f6cdfa3961/src/scr_param.c#L152

namely

    for (elem = kvtree_elem_first(value_hash);
	 elem != NULL;
	 elem = kvtree_elem_next(elem)){
      char* value = kvtree_elem_key(elem);
      if(*value == '$'){
	value = strdup(getenv(value+1));
	elem->key = value;
      }

which overwrites the elem->key of the found value hash with the expanded value of the env variable.

This:

leaks the memory that elem->key pointed to before
modifies value_hash which is a reference to the existing hash that was found (and at least in my opinion should be modified by a "get" function)

Sep 22 '19 17:09 rhaas80

Yes, nice catch!

Perhaps we could interpolate any variables once when the tree is constructed, rather than each time we get it's value? Maybe both problems are fixed if that's done during the set operation?

Sep 23 '19 17:09 adammoody

That might be an option. I am not sure which one is most preferable for scr (and fits in best with the remainder of the code).

Another option might be to pick the ENV variable value out of (and store it in) the scr_env_hash which is already used to cache ENV variable values in case the parameter is resolved from an ENV variable (the parameter itself, not its value). Since the hash is persistent one can return the value by returning the key of the leaf it belongs to.

This would be similar to the code here:

https://github.com/LLNL/scr/blob/82a3c43d8243c61fac331b64539535f6cdfa3961/src/scr_param.c#L70

Sep 23 '19 18:09 rhaas80

Right, pulling from the environment hash is also a good idea. Let's go with whichever solution you think is the best approach.

Sep 23 '19 18:09 adammoody

This reminds me of the following PR, which was dealing with environment variables from the perl scripts.

https://github.com/LLNL/scr/pull/104/files

Would you mind reviewing this code to see if it can be integrated or reworked if needed?

We have had requests where people want to specify multiple variables within a single parameter setting, like wanting to specify a path to a job-specific cache directory with something like:

/tmp/$USER/$SLURM_JOBID or /tmp/${USER}/${SLURM_JOBID}

I don't know how robust we need to be, but supporting the syntax in the above two cases would be useful.

Sep 23 '19 18:09 adammoody

That could be a separate PR from the fix for the memory leak, but figured it's a good time to bring up in case it influences design choices.

Sep 23 '19 18:09 adammoody

Sure I can review that, it may take me until tomorrow though since I Monday tends to be my "spend all day in telecons" day.

As long as the code does not have to be fast, this should be straightforward one can just scan for a $ sign then expand the value into a new string the continue with the new string (after the expanded value unless we would like to allow recursive use of ENV values).

Sep 23 '19 19:09 rhaas80

I've been sitting on my PR for about two years, so there is no rush :-)

Stepping through to process each $var in turn sounds good to me.

Thanks, @rhaas80 .

Sep 23 '19 20:09 adammoody

@rhaas80 , I've been scanning through old tickets looking to close things out. I think this one is still a problem, right?

Jul 16 '20 05:07 adammoody