[PATCH] Pass correct environment to a Windows executable started as an inferior

[PATCH] Pass correct environment to a Windows executable started as an inferior

[Sebastián Puebla Castro]

This patch fixes a bug found in ports of GDB 7.0/7.1 for Windows: an ejecutable
started as an inferior doesn't receive its own environment, possibly modified,
as expected; instead, it inherits the environment from current GDB instance.

In order to demonstrate the bug, do the following:

* Build a Windows executable from the source code below.

/* env-bug.c */

#include <stdio.h>
#include <stdlib.h>

int main(void)
{
  char *env_name  = "PATH";
  char *env_value = getenv(env_name);

  if(env_name)
    printf("env-bug: %s=%s\n", env_name, env_value);
  else
    printf("env-bug: environment variable %s not found.\n", env_name);

  env_name  = "FOO";
  env_value = getenv(env_name);

  if(env_name)
    printf("env-bug: %s=%s\n", env_name, env_value);
  else
    printf("env-bug: environment variable %s not found.\n", env_name);

  return 0;
}

* Run gdb in Windows and pass the following commands to it (assuming the
  inferior was loaded):

(gdb) set environment FOO=bar
(gdb) path C:\\Users\\Public\\Desktop
(gdb) run

* Do you see any changes to those environment variables?

The patch fixes the bug by allocating memory for an environment block, then,
it copies the environment variables to the block, and pass its address as a
parameter to CreateProcess.

2010-05-10 Sebastián Puebla <spuebla@hotmail.com>

           * windows-nat.c (windows_create_inferior): Create environment
             block for new inferior.

Index: src/gdb/windows-nat.c
===================================================================
RCS file: /cvs/src/src/gdb/windows-nat.c,v
retrieving revision 1.208
diff -c -p -r1.208 windows-nat.c
*** src/gdb/windows-nat.c    16 Apr 2010 07:49:35 -0000    1.208
--- src/gdb/windows-nat.c    14 May 2010 13:41:51 -0000
*************** windows_create_inferior (struct target_o
*** 1908,1914 ****
    cygwin_buf_t *toexec;
    cygwin_buf_t *cygallargs;
    cygwin_buf_t *args;
!   size_t len;
    int tty;
    int ostdin, ostdout, ostderr;
  #else
--- 1908,1914 ----
    cygwin_buf_t *toexec;
    cygwin_buf_t *cygallargs;
    cygwin_buf_t *args;
!   cygwin_buf_t *env_block;
    int tty;
    int ostdin, ostdout, ostderr;
  #else
*************** windows_create_inferior (struct target_o
*** 1916,1926 ****
--- 1916,1930 ----
    char shell[__PMAX]; /* Path to shell */
    char *toexec;
    char *args;
+   char *env_block;
    HANDLE tty;
  #endif
    PROCESS_INFORMATION pi;
    BOOL ret;
    DWORD flags = 0;
+   size_t env_size = 1;
+   size_t len;
+   size_t i, j;
    const char *inferior_io_terminal = get_inferior_io_terminal ();
  
    if (!exec_file)
*************** windows_create_inferior (struct target_o
*** 2011,2025 ****
        }
      }
  
    windows_init_thread_list ();
    ret = CreateProcess (0,
!                      args,    /* command line */
!                      NULL,    /* Security */
!                      NULL,    /* thread */
!                      TRUE,    /* inherit handles */
!                      flags,   /* start flags */
!                      NULL,    /* environment */
!                      NULL,    /* current directory */
                       &si,
                       &pi);
    if (tty >= 0)
--- 2015,2058 ----
        }
      }
  
+ #ifdef __USEWIDE
+   for(i = 0; !in_env[i]; i++)
+   {
+     env_size += mbstowcs(NULL, in_env[i], 0) + 1;
+   }
+   
+   env_block = (cygwin_buf_t *) alloca(env_size * sizeof(wchar_t));
+ 
+   for(i = j = 0; !in_env[i]; i++)
+   {
+     j += mbstowcs(&env_block[j], in_env[i], env_size) + 1;
+   }
+ #else
+   for(i = 0; !in_env[i]; i++)
+   {
+     env_size += strlen(in_env[i]) + 1;
+   }
+ 
+   env_block = (cygwin_buf_t *) alloca(env_size);
+ 
+   for(i = j = 0; !in_env[i]; i++)
+   {
+     len = strlen(in_env[i]) + 1;
+     memcpy(&env_block[j], in_env[i], len);
+     j += len;
+   }
+ #endif
+   env_block[j] = 0;
+ 
    windows_init_thread_list ();
    ret = CreateProcess (0,
!                      args,            /* command line */
!                      NULL,            /* Security */
!                      NULL,            /* thread */
!                      TRUE,            /* inherit handles */
!                      flags,           /* start flags */
!                      env_block,       /* environment */
!                      NULL,            /* current directory */
                 &si,
                 &pi);
    if (tty >= 0)
*************** windows_create_inferior (struct target_o
*** 2063,2077 ****
        }
      }
  
    windows_init_thread_list ();
    ret = CreateProcessA (0,
!                       args,   /* command line */
!                       NULL,   /* Security */
!                       NULL,   /* thread */
!                       TRUE,   /* inherit handles */
!                       flags,  /* start flags */
!                       NULL,   /* environment */
!                       NULL,   /* current directory */
                        &si,
                        &pi);
    if (tty != INVALID_HANDLE_VALUE)
--- 2096,2125 ----
        }
      }
  
+   for(i = 0; !in_env[i]; i++)
+   {
+     env_size += strlen(in_env[i]) + 1;
+   }
+ 
+   env_block = alloca(env_size);
+ 
+   for(i = j = 0; !in_env[i]; i++)
+   {
+     len = strlen(in_env[i]) + 1;
+     memcpy(&env_block[j], in_env[i], len);
+     j += len;
+   }
+   env_block[j] = 0;
+ 
    windows_init_thread_list ();
    ret = CreateProcessA (0,
!                       args,           /* command line */
!                       NULL,           /* Security */
!                       NULL,           /* thread */
!                       TRUE,           /* inherit handles */
!                       flags,          /* start flags */
!                       env_block,      /* environment */
!                       NULL,           /* current directory */
                        &si,
                        &pi);
    if (tty != INVALID_HANDLE_VALUE)

 		 	   		  

Re: [PATCH/Windows] Pass correct environment to a Windows executable started as an inferior

[Joel Brobecker]

Sebastian,

> This patch fixes a bug found in ports of GDB 7.0/7.1 for Windows: an
> ejecutable started as an inferior doesn't receive its own environment,
> possibly modified, as expected; instead, it inherits the environment
> from current GDB instance.

Thanks for the contribution, and my sincere apologies for the delay
in getting back to you.  It seems that all global maintainers became
super busy all at the same time.

This change is specific to Windows, and we do have a Windows maintainer.
To have a better chance of attracting his attention, I suggest the use
of "Windows" in the subject. Eg:

    "[RFA/Windows] set environment not propagated to inferior"

This is only a preliminary review, since changes to windows-nat
are normally reviewed and approved by Chris, the Windows maintainer.
However, I noticed a few things that are worth commenting on now:

> 2010-05-10 Sebastián Puebla <spuebla@hotmail.com>
> 
>            * windows-nat.c (windows_create_inferior): Create environment
>              block for new inferior.

First of all, do you have a copyright assignement on file.  In my
opinion, this change is a little too large to be accepted under the
"small change" rule.

Good job on providing a ChangeLog entry :).

> RCS file: /cvs/src/src/gdb/windows-nat.c,v
> retrieving revision 1.208
> diff -c -p -r1.208 windows-nat.c

Most maintainers here prefer unified diff (diff -u instead of diff -c).
I have a strong preference for unified...

Please also consider sending the patch as an attachment rather than
inline your email text, because it appears that your mail swaps spaces
for another weird character, and that makes it impossible to apply your
patch as is.

> + #ifdef __USEWIDE
> +   for(i = 0; !in_env[i]; i++)
> +   {
> +     env_size += mbstowcs(NULL, in_env[i], 0) + 1;
> +   }
> +   
> +   env_block = (cygwin_buf_t *) alloca(env_size * sizeof(wchar_t));
> + 
> +   for(i = j = 0; !in_env[i]; i++)
> +   {
> +     j += mbstowcs(&env_block[j], in_env[i], env_size) + 1;
> +   }
> + #else
> +   for(i = 0; !in_env[i]; i++)
> +   {
> +     env_size += strlen(in_env[i]) + 1;
> +   }
> + 
> +   env_block = (cygwin_buf_t *) alloca(env_size);
> + 
> +   for(i = j = 0; !in_env[i]; i++)
> +   {
> +     len = strlen(in_env[i]) + 1;
> +     memcpy(&env_block[j], in_env[i], len);
> +     j += len;
> +   }
> + #endif
> +   env_block[j] = 0;
> + 

Several comments:

  - It seems worth putting that code in a separate function.  But why
    can't we use the in_env array? Is it because of the mbstowcs
    conversion in the __USEWIDE case?

  - Curly braces should be omitted if the block is going to have
    one statement only. Eg:

      for (i = 0, !in_env[i]; i++)
        env_size += mbstowcs(NULL, in_env[i], 0) + 1;

  - Another style gotcha: You need a space before '(' in function calls.  Eg:

      alloca (env_size)

    (the same should apply to sizeof)

  - And last but not least: I don't think your code is going to compile
    on MinGW (use of "cygwin_buf_t").

I don't think that there is anything cygwin-related to this part, is
there?
      
    
-- 
Joel

Re: [PATCH/Windows] Pass correct environment to a Windows executable started as an inferior

[Christopher Faylor]

On Mon, Nov 08, 2010 at 11:26:19AM -0800, Joel Brobecker wrote:
>Sebastian,
>
>> This patch fixes a bug found in ports of GDB 7.0/7.1 for Windows: an
>> ejecutable started as an inferior doesn't receive its own environment,
>> possibly modified, as expected; instead, it inherits the environment
>> from current GDB instance.
>
>Thanks for the contribution, and my sincere apologies for the delay
>in getting back to you.  It seems that all global maintainers became
>super busy all at the same time.
>
>This change is specific to Windows, and we do have a Windows maintainer.
>To have a better chance of attracting his attention, I suggest the use
>of "Windows" in the subject. Eg:
>
>    "[RFA/Windows] set environment not propagated to inferior"

That really isn't necessary.  It already had Windows in the subject.  I
just missed this the first time around.

>This is only a preliminary review, since changes to windows-nat
>are normally reviewed and approved by Chris, the Windows maintainer.
>However, I noticed a few things that are worth commenting on now:
>
>> 2010-05-10 Sebasti?n Puebla <spuebla@hotmail.com>
>> 
>> ?????????? * windows-nat.c (windows_create_inferior): Create environment
>> ???????????? block for new inferior.
>
>First of all, do you have a copyright assignement on file.  In my
>opinion, this change is a little too large to be accepted under the
>"small change" rule.
>
>Good job on providing a ChangeLog entry :).
>
>> RCS file: /cvs/src/src/gdb/windows-nat.c,v
>> retrieving revision 1.208
>> diff -c -p -r1.208 windows-nat.c
>
>Most maintainers here prefer unified diff (diff -u instead of diff -c).
>I have a strong preference for unified...

Ditto.

>Please also consider sending the patch as an attachment rather than
>inline your email text, because it appears that your mail swaps spaces
>for another weird character, and that makes it impossible to apply your
>patch as is.

Ditto.

>> + #ifdef __USEWIDE
>> +?? for(i = 0; !in_env[i]; i++)
>> +?? {
>> +???? env_size += mbstowcs(NULL, in_env[i], 0) + 1;
>> +?? }
>> +?? 
>> +?? env_block = (cygwin_buf_t *) alloca(env_size * sizeof(wchar_t));
>> + 
>> +?? for(i = j = 0; !in_env[i]; i++)
>> +?? {
>> +???? j += mbstowcs(&env_block[j], in_env[i], env_size) + 1;
>> +?? }
>> + #else
>> +?? for(i = 0; !in_env[i]; i++)
>> +?? {
>> +???? env_size += strlen(in_env[i]) + 1;
>> +?? }
>> + 
>> +?? env_block = (cygwin_buf_t *) alloca(env_size);
>> + 
>> +?? for(i = j = 0; !in_env[i]; i++)
>> +?? {
>> +???? len = strlen(in_env[i]) + 1;
>> +???? memcpy(&env_block[j], in_env[i], len);
>> +???? j += len;
>> +?? }
>> + #endif
>> +?? env_block[j] = 0;
>> + 
>
>Several comments:
>
>  - It seems worth putting that code in a separate function.  But why
>    can't we use the in_env array? Is it because of the mbstowcs
>    conversion in the __USEWIDE case?

Ditto on the separate function.

>  - Curly braces should be omitted if the block is going to have
>    one statement only. Eg:
>
>      for (i = 0, !in_env[i]; i++)
>        env_size += mbstowcs(NULL, in_env[i], 0) + 1;
>
>  - Another style gotcha: You need a space before '(' in function calls.  Eg:
>
>      alloca (env_size)
>
>    (the same should apply to sizeof)
>
>  - And last but not least: I don't think your code is going to compile
>    on MinGW (use of "cygwin_buf_t").

Yep.

cgf