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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

Hi Sergio,

On 2017-07-27 05:35, Sergio Durigan Junior wrote:
> Changes from v1:
>
> - Implement suggestions by Simon.
>
> - Improve gdb_environ::unset (use std::remove).
>
> - Implement the ability to unset environment variables in the remote
>   target.
>
> - Implement hex2str and str2hex.
>
> - Extend unittest, testcase and documentation.
>
> This version of the patch extends the first version/idea, which only
> allowed environment variables to be set in the remote target.  Now it
> is also possible to unset them.

Awesome!

> 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.  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.

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

> The QEnvironmentHexEncoded packet is inspired on LLDB's extensions to
> the RSP.  Details about it can be seen here:
>
>
> <https://raw.githubusercontent.com/llvm-mirror/lldb/master/docs/lldb-gdb-remote.txt>
>
> I decided not to implement the QEnvironment packet because it is
> considered deprecated by LLDB.  This packet, on LLDB, serves the same
> purpose of QEnvironmentHexEncoded, but sends the information using a
> plain text, non-hex-encoded string.
>
> The other two packets are new.
>
> This patch also includes updates to the documentation, testsuite, and
> unit tests, without introducing regressions.
>
> gdb/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* NEWS (Changes since GDB 8.0): Add entry mentioning new support
> 	for setting/unsetting environment variables on the remote target.
> 	(New remote packets): Add entries for QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset.
> 	* common/environ.c (gdb_environ::operator=): Extend method to
> 	handle m_user_set_env_list and m_user_unset_env_list.
> 	(gdb_environ::clear): Likewise.
> 	(match_var_in_string): Change type of first parameter from 'char
> 	*' to 'const char *'.
> 	(gdb_environ::set): Likewise.
> 	(gdb_environ::unset): Likewise.
> 	(gdb_environ::clear_user_set_env): New method.
> 	(gdb_environ::user_set_envp): Likewise.
> 	(gdb_environ::user_unset_envp): Likewise.
> 	* common/environ.h (gdb_environ): Handle m_user_set_env_list and
> 	m_user_unset_env_list on move constructor/assignment.
> 	(unset): Add new default parameter 'update_unset_list = true'.
> 	(clear_user_set_env): New method.
> 	(user_set_envp): Likewise.
> 	(user_unset_envp): Likewise.
> 	(m_user_set_env_list): New vector.
> 	(m_user_unset_env_list): Likewise.
> 	* common/rsp-low.c (hex2str): New function.
> 	(str2hex): Likewise.
> 	* common/rsp-low.c (hex2str): New prototype.
> 	(str2hex): Likewise.
> 	* remote.c: Include "environ.h". Add QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset.
> 	(remote_protocol_features): Add QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset packets.
> 	(send_environment_packets): New function.
> 	(extended_remote_environment_support): Likewise.
> 	(extended_remote_create_inferior): Call
> 	extended_remote_environment_support.
> 	(_initialize_remote): Add QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset packet configs.
> 	* unittests/environ-selftests.c (run_tests): Add tests for
> 	m_user_set_env_list and m_user_unset_env_list.
>
> gdb/gdbserver/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* server.c (handle_general_set): Handle QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset packets.
> 	(handle_query): Inform remote that QEnvironmentHexEncoded,
> 	QEnvironmentUnset and QEnvironmentReset are supported.
>
> gdb/doc/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* gdb.texinfo (set environment): Add @anchor.  Explain that
> 	environment variables set by the user are sent to GDBserver.
> 	(unset environment): Likewise, but for unsetting variables.
> 	(Connecting) <Remote Packet>: Add "environment-hex-encoded",
> 	"QEnvironmentHexEncoded", "environment-unset", "QEnvironmentUnset",
> 	"environment-reset" and "QEnvironmentReset" to the table.
> 	(Remote Protocol) <QEnvironmentHexEncoded, QEnvironmentUnset,
> 	QEnvironmentReset>: New item, explaining the packet.
>
> gdb/testsuite/ChangeLog:
> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>
> 	* gdb.base/share-env-with-gdbserver.c: New file.
> 	* gdb.base/share-env-with-gdbserver.exp: Likewise.
> ---
>  gdb/NEWS                                           |  24 ++
>  gdb/common/environ.c                               | 122 ++++++++--
>  gdb/common/environ.h                               |  25 +-
>  gdb/common/rsp-low.c                               |  37 +++
>  gdb/common/rsp-low.h                               |   8 +
>  gdb/doc/gdb.texinfo                                | 116 ++++++++++
>  gdb/gdbserver/server.c                             |  73 +++++-
>  gdb/remote.c                                       |  82 +++++++
>  gdb/testsuite/gdb.base/share-env-with-gdbserver.c  |  40 ++++
>  .../gdb.base/share-env-with-gdbserver.exp          | 254 
> +++++++++++++++++++++
>  gdb/unittests/environ-selftests.c                  |  55 +++++
>  11 files changed, 819 insertions(+), 17 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.base/share-env-with-gdbserver.c
>  create mode 100644 gdb/testsuite/gdb.base/share-env-with-gdbserver.exp
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 9cd1df1..4349cce 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,18 @@
>
>  *** Changes since GDB 8.0
>
> +* On Unix systems, GDB now supports transmitting environment variables
> +  that are to be set or unset to GDBserver.  These variables will
> +  affect the environment to be passed to the remote inferior.
> +
> +  To inform GDB of environment variables that are to be transmitted to
> +  GDBserver, use the "set environment" command.  Only user set
> +  environment variables are sent to GDBserver.
> +
> +  To inform GDB of environment variables that are to be unset before
> +  the remote inferior is started by the GDBserver, use the "unset
> +  environment" command.
> +
>  * On Unix systems, GDBserver now does globbing expansion and variable
>    substitution in inferior command line arguments.
>
> @@ -14,6 +26,18 @@
>
>  * New remote packets
>
> +QEnvironmentHexEncoded
> +  Inform GDBserver of an environment variable that is to be passed to
> +  the inferior when starting it.
> +
> +QEnvironmentUnset
> +  Inform GDBserver of an environment variable that is to be unset
> +  before starting the remote inferior.
> +
> +QEnvironmentReset
> +  Inform GDBserver that the environment should be reset (i.e.,
> +  user-set environment variables should be unset).
> +
>  QStartupWithShell
>    Indicates whether the inferior must be started with a shell or not.
>
> 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.

>  }
> @@ -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);
  }

> +
> +  /* 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).


> 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);

> +
>  /* 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);


> --- 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.

> +      /* 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"?


> +/* 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")?


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

Thanks!

Simon