Re: [PATCH v5] C++ify gdb/common/environ.c

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-06-17 00:23, Sergio Durigan Junior wrote:
>  void
> -set_in_environ (struct gdb_environ *e, const char *var, const char 
> *value)
> +gdb_environ::set (const char *var, const char *value)
>  {
> -  int i;
> -  int len = strlen (var);
> -  char **vector = e->vector;
> -  char *s;
> -
> -  for (i = 0; (s = vector[i]) != NULL; i++)
> -    if (strncmp (s, var, len) == 0 && s[len] == '=')
> -      break;
> +  /* We have to unset the variable in the vector if it exists.  */
> +  unset (var);
> 
> -  if (s == 0)
> -    {
> -      if (i == e->allocated)
> -	{
> -	  e->allocated += 10;
> -	  vector = (char **) xrealloc ((char *) vector,
> -				       (e->allocated + 1) * sizeof (char *));
> -	  e->vector = vector;
> -	}
> -      vector[i + 1] = 0;
> -    }
> -  else
> -    xfree (s);
> -
> -  s = (char *) xmalloc (len + strlen (value) + 2);
> -  strcpy (s, var);
> -  strcat (s, "=");
> -  strcat (s, value);
> -  vector[i] = s;
> -
> -  /* This used to handle setting the PATH and GNUTARGET variables
> -     specially.  The latter has been replaced by "set gnutarget"
> -     (which has worked since GDB 4.11).  The former affects searching
> -     the PATH to find SHELL, and searching the PATH to find the
> -     argument of "symbol-file" or "exec-file".  Maybe we should have
> -     some kind of "set exec-path" for that.  But in any event, having
> -     "set env" affect anything besides the inferior is a bad idea.
> -     What if we want to change the environment we pass to the program
> -     without afecting GDB's behavior?  */
> -
> -  return;
> +  /* Insert the element before the last one, which is always NULL.  */
> +  m_environ_vector.insert (m_environ_vector.end () - 1,
> +			   concat (var, "=", value, NULL));

The breaks if we have just constructed an empty gdb_environ object, as 
the vector is completely empty (no terminating NULL).  So we'd need some 
kind of check before that, if the vector is empty, add a NULL element...

I actually preferred the option of adding the NULL element to the vector 
in the gdb_environ constructor, since it allows always having the vector 
in a consistent state.  I don't think that avoiding that heap allocation 
is worth the complexity it adds to the code (unless we can prove 
otherwise by memory usage profiling).

> +static void
> +run_tests ()
> +{
> +  if (setenv ("GDB_SELFTEST_ENVIRON", "1", 1) != 0)
> +    error ("Could not set environment variable for testing.");
> +
> +  gdb_environ env;
> +
> +  SELF_CHECK (env.envp ()[0] == NULL);
> +
> +  SELF_CHECK (env.get ("PWD") == NULL);

If you add

   env.set ("PWD", "/home");

you should see a crash.

Simon