Re: [PATCH v2] Implement the ability to set/unset environment variables to GDBserver when starting the inferior

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Thursday, July 27 2017, Simon Marchi wrote:

> Hi Sergio,
>
> On 2017-07-27 05:35, Sergio Durigan Junior wrote:
>> User-set environment variables are only the variables that are
>> explicitly set by the user, using the 'set environment' command.  This
>> means that variables that were already present in the environment when
>> starting GDB/GDBserver are not transmitted/considered by this feature.
>>
>> User-unset environment variables are variables that are explicitly
>> unset by the user, using the 'unset environment' command.
>>
>> The idea behind this patch is to store user-set and user-unset
>> environment variables in two separate vectors, both part of
>> gdb_environ.  Then, when extended_remote_create_inferior is preparing
>> to start the inferior, it will iterate over the two lists and
>> set/unset variables accordingly.  Three new packets are introduced:
>>
>> - QEnvironmentHexEncoded, which is used to set environment variables,
>>   and contains an hex-encoded string in the format "VAR=VALUE" (VALUE
>>   can be empty if the user set a variable with a null value, by doing
>>   'set environment VAR=').
>>
>> - QEnvironmentUnset, which is used to unset environment variables, and
>>   contains an hex-encoded string in the format "VAR".
>>
>> - QEnvironmentReset, which is always the first packet to be
>>   transmitted, and is used to reset (i.e., unset any user-set
>>   environment variable) the environment.
>
> There is this use case for which the behavior is different between
> native and remote, related to unset
>
> native:
>
> (gdb inf1) file /usr/bin/env
> (gdb inf1) unset environment DISPLAY
> (gdb inf1) r  # DISPLAY is not there
> (gdb inf1) add-inferior -exec /usr/bin/env
> (gdb inf1) inferior 2
> (gdb inf2) r  # DISPLAY is there
>
> remote:
>
> (gdb inf1) tar ext :1234
> (gdb inf1) file /usr/bin/env
> (gdb inf1) set remote exec-file /usr/bin/env
> (gdb inf1) unset environment DISPLAY
> (gdb inf1) r  # DISPLAY is not there
> (gdb inf1) add-inferior -exec /usr/bin/env
> (gdb inf1) inferior 2
> (gdb inf2) set remote exec-file /usr/bin/env
> (gdb inf2) r  # DISPLAY is not there
>
> I think that's because in GDB, we make a new pristine copy of the host
> environment for every inferior, which we don't in gdbserver.

Thanks for the review, Simon.

Yes, you're right, these cases are currently different because of the
way we handle the environment on GDB and gdbserver.  On gdbserver we
have 'our_environ', which is a global declared at server.c and that is
passed to all inferiors being started.

> The way I understand the QEnvironmentReset is that the remote agent
> (gdbserver) should forget any previous modification to the environment
> made using QEnvironmentHexEncoded and QEnvironmentUnset and return the
> environment to its original state, when it was launched.  This should
> allow supporting the use case above.  To implement that properly, we
> would need to keep a copy of gdbserver's initial environment, which we
> could revert to when receiving a QEnvironmentReset.

Yes, and we already do that on gdbserver; see the 'our_environ' global.

> In any case, I just want to make sure that the packets we introduce
> are not the things that limit us.

Sorry, I'm not sure I understood what you have in mind.  Could you
explain in what ways we'd be limited by the new packets?

> [...]
>> diff --git a/gdb/common/environ.c b/gdb/common/environ.c
>> index 698bda3..59b081f 100644
>> --- a/gdb/common/environ.c
>> +++ b/gdb/common/environ.c
>> @@ -30,8 +30,12 @@ gdb_environ::operator= (gdb_environ &&e)
>>      return *this;
>>
>>    m_environ_vector = std::move (e.m_environ_vector);
>> +  m_user_set_env_list = std::move (e.m_user_set_env_list);
>> +  m_user_unset_env_list = std::move (e.m_user_unset_env_list);
>>    e.m_environ_vector.clear ();
>>    e.m_environ_vector.push_back (NULL);
>> +  e.m_user_set_env_list.clear ();
>> +  e.m_user_unset_env_list.clear ();
>>    return *this;
>>  }
>>
>> @@ -63,6 +67,10 @@ gdb_environ::clear ()
>>    for (char *v : m_environ_vector)
>>      xfree (v);
>>    m_environ_vector.clear ();
>> +  m_user_set_env_list.clear ();
>> +  for (const char *v : m_user_unset_env_list)
>> +    xfree ((void *) v);
>> +  m_user_unset_env_list.clear ();
>>    /* Always add the NULL element.  */
>>    m_environ_vector.push_back (NULL);
>
> I'd keep these last two lines just after the "m_environ_vector.clear
> ()", since they're related.

Hm, good, I was trying to keep them close but I forgot this one.  Fixed.

>
>>  }
>> @@ -72,7 +80,7 @@ gdb_environ::clear ()
>>     if it contains, false otherwise.  */
>>
>>  static bool
>> -match_var_in_string (char *string, const char *var, size_t var_len)
>> +match_var_in_string (const char *string, const char *var, size_t
>> var_len)
>>  {
>>    if (strncmp (string, var, var_len) == 0 && string[var_len] == '=')
>>      return true;
>> @@ -99,32 +107,104 @@ gdb_environ::get (const char *var) const
>>  void
>>  gdb_environ::set (const char *var, const char *value)
>>  {
>> +  char *fullvar = concat (var, "=", value, NULL);
>> +
>>    /* We have to unset the variable in the vector if it exists.  */
>> -  unset (var);
>> +  unset (var, false);
>>
>>    /* Insert the element before the last one, which is always NULL.  */
>> -  m_environ_vector.insert (m_environ_vector.end () - 1,
>> -			   concat (var, "=", value, NULL));
>> +  m_environ_vector.insert (m_environ_vector.end () - 1, fullvar);
>> +
>> +  /* Mark this environment variable as having been set by the user.
>> +     This will be useful when we deal with setting environment
>> +     variables on the remote target.  */
>> +  m_user_set_env_list.push_back (fullvar);
>> +
>> +  /* If this environment variable is marked as unset by the user, then
>> +     remove it from the list, because now the user wants to set
>> +     it.  */
>> +  for (std::vector<const char *>::iterator iter_user_unset
>> +	 = m_user_unset_env_list.begin ();
>> +       iter_user_unset != m_user_unset_env_list.end ();
>> +       ++iter_user_unset)
>> +    if (strcmp (var, *iter_user_unset) == 0)
>> +      {
>> +	void *v = (void *) *iter_user_unset;
>> +
>> +	m_user_unset_env_list.erase (iter_user_unset);
>> +	xfree (v);
>> +	break;
>> +      }
>>  }
>>
>>  /* See common/environ.h.  */
>>
>>  void
>> -gdb_environ::unset (const char *var)
>> +gdb_environ::unset (const char *var, bool update_unset_list)
>>  {
>>    size_t len = strlen (var);
>> +  std::vector<char *>::iterator it_env;
>>
>>    /* We iterate until '.end () - 1' because the last element is
>>       always NULL.  */
>> -  for (std::vector<char *>::iterator el = m_environ_vector.begin ();
>> -       el != m_environ_vector.end () - 1;
>> -       ++el)
>> -    if (match_var_in_string (*el, var, len))
>> -      {
>> -	xfree (*el);
>> -	m_environ_vector.erase (el);
>> -	break;
>> -      }
>> +  for (it_env = m_environ_vector.begin ();
>> +       it_env != m_environ_vector.end () - 1;
>> +       ++it_env)
>> +    if (match_var_in_string (*it_env, var, len))
>> +      break;
>> +
>> +  if (it_env == m_environ_vector.end () - 1)
>> +    {
>> +      /* No element has been found.  */
>> +      return;
>> +    }
>> +
>> +  std::vector<const char *>::iterator it_user_set_env;
>> +  char *found_var = *it_env;
>> +
>> +  it_user_set_env = std::remove (m_user_set_env_list.begin (),
>> +				 m_user_set_env_list.end (),
>> +				 found_var);
>> +  if (it_user_set_env != m_user_set_env_list.end ())
>> +    {
>> +      /* We found (and removed) the element from the user_set_env
>> +	 vector.  */
>> +      m_user_set_env_list.erase (it_user_set_env,
>> m_user_set_env_list.end ());
>> +    }
>> +
>> +  if (update_unset_list)
>> +    {
>> +      bool found_in_unset = false;
>> +
>> +      for (const char *el : m_user_unset_env_list)
>> +	if (strcmp (el, var) == 0)
>> +	  {
>> +	    found_in_unset = true;
>> +	    break;
>> +	  }
>> +
>> +      if (!found_in_unset)
>> +	m_user_unset_env_list.push_back (xstrdup (var));
>> +    }
>> +
>> +  m_environ_vector.erase (it_env);
>> +  xfree (found_var);
>> +}
>> +
>> +/* See common/environ.h.  */
>> +
>> +void
>> +gdb_environ::clear_user_set_env ()
>> +{
>> +  std::vector<const char *> copy = m_user_set_env_list;
>> +
>> +  for (const char *var : copy)
>> +    {
>> +      std::string varname (var);
>> +
>> +      varname.erase (varname.find ('='), std::string::npos);
>> +      unset (varname.c_str (), false);
>> +    }
>>  }
>>
>>  /* See common/environ.h.  */
>> @@ -134,3 +214,17 @@ gdb_environ::envp () const
>>  {
>>    return const_cast<char **> (&m_environ_vector[0]);
>>  }
>> +
>> +/* See common/environ.h.  */
>> +
>> +const std::vector<const char *> &
>> +gdb_environ::user_set_envp () const
>> +{
>> +  return m_user_set_env_list;
>> +}
>> +
>> +const std::vector<const char *> &
>> +gdb_environ::user_unset_envp () const
>> +{
>> +  return m_user_unset_env_list;
>> +}
>> diff --git a/gdb/common/environ.h b/gdb/common/environ.h
>> index 0bbb191..2038170 100644
>> --- a/gdb/common/environ.h
>> +++ b/gdb/common/environ.h
>> @@ -41,12 +41,16 @@ public:
>>
>>    /* Move constructor.  */
>>    gdb_environ (gdb_environ &&e)
>> -    : m_environ_vector (std::move (e.m_environ_vector))
>> +    : m_environ_vector (std::move (e.m_environ_vector)),
>> +      m_user_set_env_list (std::move (e.m_user_set_env_list)),
>> +      m_user_unset_env_list (std::move (e.m_user_unset_env_list))
>>    {
>>      /* Make sure that the moved-from vector is left at a valid
>>         state (only one NULL element).  */
>>      e.m_environ_vector.clear ();
>>      e.m_environ_vector.push_back (NULL);
>> +    e.m_user_set_env_list.clear ();
>> +    e.m_user_unset_env_list.clear ();
>>    }
>>
>>    /* Move assignment.  */
>> @@ -68,14 +72,31 @@ public:
>>    void set (const char *var, const char *value);
>>
>>    /* Unset VAR in environment.  */
>> -  void unset (const char *var);
>> +  void unset (const char *var, bool update_unset_list = true);
>
> Can you document the new parameter?
>
> If this parameter is only useful internally, you could make it a
> private method, so that it's not exposed to the outside:
>
> private:
>   void unset (const char *var, bool update_unset_list);
>
> and have the public version with a single parameter call it:
>
>   void gdb_environ::unset (const char *var)
>   {
>     unset (var, true);
>   }

Good idea!  I was trying to avoid adding a new method for that, but
overloading unset seems like a cleaner approach.

>
>> +
>> +  /* Iterate through M_USER_UNSET_ENV_LIST and unset all variables.
>> */
>> +  void clear_user_set_env ();
>
> I wouldn't put the "Iterate through M_USER_UNSET_ENV_LIST" in the
> comment, since that's implementation details.  The function
> documentation should focus on the visible effects or goals of the
> function (i.e. remove the user set/unset variables in that
> environment).

Good point.  I've updated the comment to:

  /* Unset all variables that were previously set by the user, i.e.,
     that were added by calling the SET method.  */
  void clear_user_set_env ();

>> diff --git a/gdb/common/rsp-low.h b/gdb/common/rsp-low.h
>> index b57f58b..0c22d4f 100644
>> --- a/gdb/common/rsp-low.h
>> +++ b/gdb/common/rsp-low.h
>> @@ -52,6 +52,10 @@ extern char *unpack_varlen_hex (char *buff,
>> ULONGEST *result);
>>
>>  extern int hex2bin (const char *hex, gdb_byte *bin, int count);
>>
>> +/* Like hex2bin, but work on std::string.  */
>> +
>> +extern int hex2str (const std::string &hex, std::string &str);
>
> The variables passed to this hex parameter are char* pointing inside
> the received packet in server.c.  Therefore, to avoid unnecessary
> copies and construction of string, I'd leave hex as a const char *.
> The return value doesn't seem needed either.  What about this
> signature?
>
> extern std::string hex2str (const char *hex);

Fair enough, I like the idea.  Update accordingly.

>
>> +
>>  /* Convert some bytes to a hexadecimal representation.  BIN holds the
>>     bytes to convert.  COUNT says how many bytes to convert.  The
>>     resulting characters are stored in HEX, followed by a NUL
>> @@ -59,6 +63,10 @@ extern int hex2bin (const char *hex, gdb_byte *bin,
>> int count);
>>
>>  extern int bin2hex (const gdb_byte *bin, char *hex, int count);
>>
>> +/* Like bin2hex, but work on std::strings.  */
>> +
>> +extern int str2hex (const std::string &str, std::string &hex);
>> +
>
> Similar thing here, what we pass to str is a const char*, so it leads
> to an unnecessary std::string construction.  Also, the interface of
> the function is not well-suited to encode arbitrary binary data, which
> could any byte.  One could use
>
>   str2hex (std::string (p, count), hex);
>
> but again why copy the data in the first place? So what about this:
>
> extern std::string bin2hex (const char *bin, int count);

Not sure if it was a typo or if you really meant to propose overloading
the bin2hex method and have another version of it that returns a
std::string, but I like the idea.  I don't think we need to treat the
first parameter as a 'const char *'; TBH, treating it like a 'const
gdb_byte *' (just like the original version does) is more clear.  So I
went ahead and implemented it this way.

>
>> --- a/gdb/gdbserver/server.c
>> +++ b/gdb/gdbserver/server.c
>> @@ -631,6 +631,75 @@ handle_general_set (char *own_buf)
>>        return;
>>      }
>>
>> +  if (startswith (own_buf, "QEnvironmentReset"))
>> +    {
>> +      our_environ.clear_user_set_env ();
>> +
>> +      write_ok (own_buf);
>> +      return;
>> +    }
>> +
>> +  if (startswith (own_buf, "QEnvironmentHexEncoded:"))
>> +    {
>> +      const char *p = own_buf + sizeof ("QEnvironmentHexEncoded:")
>> - 
>> 1;
>
> You can also use strlen to avoid having to do -1, but either is fine
> with me.

I personally like using sizeof here and avoiding the call to strlen.

>> +      /* The final form of the environment variable.  FINAL_VAR will
>> +	 hold the 'VAR=VALUE' format.  */
>> +      std::string final_var;
>> +      std::string var_name, var_value;
>> +
>> +      if (remote_debug)
>> +	debug_printf (_("[QEnvironmentHexEncoded received '%s']\n"), p);
>> +
>> +      hex2str (p, final_var);
>> +
>> +      if (remote_debug)
>> +	{
>> +	  debug_printf (_("[Environment variable to be set: '%s']\n"),
>> +			final_var.c_str ());
>> +	  debug_flush ();
>> +	}
>> +
>> +      size_t pos = final_var.find ('=');
>> +      if (pos == std::string::npos)
>> +	{
>> +	  warning (_("Unknown environment variable '%s' from remote side."),
>
> I don't find this error message very clear, maybe more something like
> "Unexpected format"?

Sure thing.

>
>> +/* Helper function to handle the QEnvironment* packets.  */
>> +
>> +static void
>> +extended_remote_environment_support (struct remote_state *rs)
>> +{
>> +  if (packet_support (PACKET_QEnvironmentReset) != PACKET_DISABLE)
>> +    {
>> +      static const char qenvreset[] = "QEnvironmentReset";
>> +
>> +      putpkt (qenvreset);
>
> Could this be directly
>
>   putpkt ("QEnvironmentReset")?

Yep, totally.

> I haven't looked at everything yet and it's getting late here, I'll
> try to continue tomorrow.

Thanks for the message.  I'll now reply to your other review :-).

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/