Re: [PATCH] kernel32: Don't use putenv(3) on temporary strings.

[Ken Thomases]

The putenv() calls occur in a forked grandchild process just before an exec(). The strings are sure to survive the current process image.  After that, it's the kernel's problem.

Regards,
Ken

On Jun 10, 2012, at 11:47 AM, Charles Davis wrote:
> The Darwin manpage for putenv() warns that it no longer makes a copy of
> its input, but directly stores the pointer in environ. Wine was passing
> stack- and heap-allocated strings to putenv(), which could have caused a
> crash. We were very lucky it didn't. Instead, use setenv(3), which still
> copies its inputs.
>
> Found while tracking down an insidious bug in Clang-built optimized
> (-O2) Wine.
> ---
> dlls/kernel32/process.c |   24 +++++++++++-------------
> 1 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/dlls/kernel32/process.c b/dlls/kernel32/process.c
> index 42716c2..1ec3e3d 100644
> --- a/dlls/kernel32/process.c
> +++ b/dlls/kernel32/process.c
> @@ -1741,17 +1741,14 @@ static const char *get_alternate_loader( char **ret_env )
>         int len = strlen( loader_env );
>         if (!is_win64)
>         {
> -            if (!(env = HeapAlloc( GetProcessHeap(), 0, sizeof("WINELOADER=") + len + 2 ))) return NULL;
> -            strcpy( env, "WINELOADER=" );
> -            strcat( env, loader_env );
> +            if (!(env = HeapAlloc( GetProcessHeap(), 0, len + 2 ))) return NULL;
> +            strcpy( env, loader_env );
>             strcat( env, "64" );
>         }
>         else
>         {
> -            if (!(env = HeapAlloc( GetProcessHeap(), 0, sizeof("WINELOADER=") + len ))) return NULL;
> -            strcpy( env, "WINELOADER=" );
> -            strcat( env, loader_env );
> -            len += sizeof("WINELOADER=") - 1;
> +            if (!(env = HeapAlloc( GetProcessHeap(), 0, len ))) return NULL;
> +            strcpy( env, loader_env );
>             if (!strcmp( env + len - 2, "64" )) env[len - 2] = 0;
>         }
>         if (!loader)
> @@ -1860,14 +1857,14 @@ static pid_t exec_loader( LPCWSTR cmd_line, unsigned int flags, int socketfd,
>             /* Reset signals that we previously set to SIG_IGN */
>             signal( SIGPIPE, SIG_DFL );
>
> -            sprintf( socket_env, "WINESERVERSOCKET=%u", socketfd );
> -            sprintf( preloader_reserve, "WINEPRELOADRESERVE=%lx-%lx",
> +            sprintf( socket_env, "%u", socketfd );
> +            sprintf( preloader_reserve, "%lx-%lx",
>                      (unsigned long)binary_info->res_start, (unsigned long)binary_info->res_end );
>
> -            putenv( preloader_reserve );
> -            putenv( socket_env );
> -            if (winedebug) putenv( winedebug );
> -            if (wineloader) putenv( wineloader );
> +            setenv( "WINESERVERSOCKET", socket_env, 1 );
> +            setenv( "WINEPRELOADRESERVE", preloader_reserve, 1 );
> +            if (winedebug) setenv( "WINEDEBUG", winedebug, 1 );
> +            if (wineloader) setenv( "WINELOADER", wineloader, 1 );
>             if (unixdir) chdir(unixdir);
>
> if (argv)
> @@ -1976,6 +1973,7 @@ static BOOL create_process( HANDLE hFile, LPCWSTR filename, LPWSTR cmd_line, LPW
>         static const WCHAR WINEDEBUG[] = {'W','I','N','E','D','E','B','U','G','=',0};
>         if (!winedebug && !strncmpW( env_end, WINEDEBUG, sizeof(WINEDEBUG)/sizeof(WCHAR) - 1 ))
>         {
> +            env_end += sizeof(WINEDEBUG)/sizeof(WCHAR) - 1;
>             DWORD len = WideCharToMultiByte( CP_UNIXCP, 0, env_end, -1, NULL, 0, NULL, NULL );
>             if ((winedebug = HeapAlloc( GetProcessHeap(), 0, len )))
>                 WideCharToMultiByte( CP_UNIXCP, 0, env_end, -1, winedebug, len, NULL, NULL );
> -- 
> 1.7.5.4