Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows getenv() and GetEnvironmentVariable() are not the same #4774

Closed
jay opened this issue Jan 2, 2020 · 7 comments
Closed

Windows getenv() and GetEnvironmentVariable() are not the same #4774

jay opened this issue Jan 2, 2020 · 7 comments
Labels
Windows Windows-specific

Comments

@jay
Copy link
Member

jay commented Jan 2, 2020

@cmb69 reported on the mailing list that Windows environment variables set via SetEnvironmentVariable are visible to GetEnvironmentVariable but not to getenv. I've confirmed this.

Some builds of Windows php for their own API putenv call SetEnvironmentVariable instead of putenv. That can lead to a bug. In that case a user set some PROXY variables via php's putenv and when curl retrieved them via getenv they were not found. (Christoph please correct this if I misinterpreted)

Related issues

cmb69's e-mail also points out getenv is not thread safe but I assume he means if the environment block is modified. This is a well known issue for various runtimes and perhaps we should warn about using putenv in the thread safety guidelines.

The curl tool's GetEnv already uses GetEnvironmentVariable instead of getenv and there is a comment "Don't use getenv(); it doesn't find variable added after program was started". Frankly that doesn't make a lot of sense, and as far as I can tell is not true (set a variable with putenv and it's immediately avaiable via getenv). It's possible the author was mistaken and had the same issue discussed here.

curl/src/tool_homedir.c

Lines 32 to 42 in 2e9b725

static char *GetEnv(const char *variable, char do_expand)
{
char *env = NULL;
#ifdef WIN32
char buf1[1024], buf2[1024];
DWORD rc;
/* Don't use getenv(); it doesn't find variable added after program was
* started. Don't accept truncated results (i.e. rc >= sizeof(buf1)). */
rc = GetEnvironmentVariableA(variable, buf1, sizeof(buf1));

Remedy

To completely remedy this in curl would require a bit of work since we call getenv multiple places without having to worry about memory management, like we would with curl_getenv. We could at least address the php bug by improving on curl_getenv to obtain variables via GetEnvironmentVariable instead of getenv. The easiest way to do that is c&p of GetEnv from the tool and use it in the lib:

diff --git a/lib/getenv.c b/lib/getenv.c
index e444a6a..9ca8f19 100644
--- a/lib/getenv.c
+++ b/lib/getenv.c
@@ -34,17 +34,31 @@ char *GetEnv(const char *variable)
   (void)variable;
   return NULL;
 #else
+  char *env = NULL;
 #ifdef WIN32
-  char env[4096];
-  char *temp = getenv(variable);
-  env[0] = '\0';
-  if(temp != NULL)
-    ExpandEnvironmentStringsA(temp, env, sizeof(env));
-  return (env[0] != '\0')?strdup(env):NULL;
+  char  buf1[1024], buf2[1024];
+  DWORD rc;
+
+  /* Don't use getenv(); it doesn't find variable added after program was
+   * started. Don't accept truncated results (i.e. rc >= sizeof(buf1)).  */
+
+  rc = GetEnvironmentVariableA(variable, buf1, sizeof(buf1));
+  if(rc > 0 && rc < sizeof(buf1)) {
+    env = buf1;
+    variable = buf1;
+  }
+  if(strchr(variable, '%')) {
+    /* buf2 == variable if not expanded */
+    rc = ExpandEnvironmentStringsA(variable, buf2, sizeof(buf2));
+    if(rc > 0 && rc < sizeof(buf2) &&
+       !strchr(buf2, '%'))    /* no vars still unexpanded */
+      env = buf2;
+  }
 #else
-  char *env = getenv(variable);
-  return (env && env[0])?strdup(env):NULL;
+  /* no length control */
+  env = getenv(variable);
 #endif
+  return (env && env[0]) ? strdup(env) : NULL;
 #endif
 }

Is there an acceptable way to deduplicate functions used by both the tool and lib? I know of curlx but I understand those are deprecated, therefore if I wanted change GetEnv in the tool to call curlx_getenv (which would then call GetEnv in the lib and now has the same code) would that be acceptable?

To address this fully would require banning getenv and then replacing it with curl_getenv everywhere, and I think that's unnecessary. I'd rather note KNOWN_BUGS there may be circumstances where curl may not recognize variables set via SetEnvironmentVariable.

curl/libcurl version

curl 7.68.0-DEV (i386-pc-win32) libcurl/7.68.0-DEV

@jay jay added the Windows Windows-specific label Jan 2, 2020
@cmb69
Copy link
Contributor

cmb69 commented Jan 3, 2020

Thanks! I've not much to add, except for

cmb69's e-mail also points out getenv is not thread safe but I assume he means if the environment block is modified. This is a well known issue for various runtimes and perhaps we should warn about using putenv in the thread safety guidelines.

Yes, please at least document this in the thread safety guidelines. MSDN explicitly points out:

The _putenv and _getenv families of functions are not thread-safe. _getenv could return a string pointer while _putenv is modifying the string, causing random failures. Make sure that calls to these functions are synchronized.

POSIX doesn't mandate that getenv() has to be thread-safe either.

@bagder
Copy link
Member

bagder commented Jan 3, 2020

(removed erroneous comment)

Shouldn't you create a PR with this to see what the tests say about it?

@jay
Copy link
Member Author

jay commented Jan 4, 2020

Shouldn't you create a PR with this to see what the tests say about it?

It's a preliminary for discussion. I'm unsure on how best to proceed. I think the duplication is a crude way to fix it. I'd rather share the code between the lib and the tool. It'll do the job though.

@jay
Copy link
Member Author

jay commented Jan 24, 2020

Any opinion here? Should I turn it into a curlx or just submit it with the duplication?

@bagder
Copy link
Member

bagder commented Jan 24, 2020

I don't have any particular strong opinion either way. I think you can do what's easiest for you. The curlx method often turns out cumbersome so it isn't necessary a clear win to share code that way.

jay added a commit to jay/curl that referenced this issue Jan 29, 2020
- Deduplicate GetEnv() code.

- On Windows change ultimate call to use Windows API
  GetEnvironmentVariable() instead of C runtime getenv().

Prior to this change both libcurl and the tool had their own GetEnv
which over time diverged. Now the tool's GetEnv is a wrapper around
curl_getenv (libcurl API function which is itself a wrapper around
libcurl's GetEnv).

Furthermore this change fixes a bug in that Windows API
GetEnvironmentVariable() is called instead of C runtime getenv() to get
the environment variable since some changes aren't always visible to the
latter.

Reported-by: Christoph M. Becker

Fixes curl#4774
Closes #xxxx
@jay
Copy link
Member Author

jay commented Jan 29, 2020

@cmb69 please check if #4863 fixes your issue

@cmb69
Copy link
Contributor

cmb69 commented Jan 29, 2020

@jay, yes that would fix my issue. Thanks! I left 2 inline comments regarding implementation details.

jay added a commit to jay/curl that referenced this issue Jan 29, 2020
- Deduplicate GetEnv() code.

- On Windows change ultimate call to use Windows API
  GetEnvironmentVariable() instead of C runtime getenv().

Prior to this change both libcurl and the tool had their own GetEnv
which over time diverged. Now the tool's GetEnv is a wrapper around
curl_getenv (libcurl API function which is itself a wrapper around
libcurl's GetEnv).

Furthermore this change fixes a bug in that Windows API
GetEnvironmentVariable() is called instead of C runtime getenv() to get
the environment variable since some changes aren't always visible to the
latter.

Reported-by: Christoph M. Becker

Fixes curl#4774
Closes #xxxx
jay added a commit to jay/curl that referenced this issue Jan 29, 2020
- Deduplicate GetEnv() code.

- On Windows change ultimate call to use Windows API
  GetEnvironmentVariable() instead of C runtime getenv().

Prior to this change both libcurl and the tool had their own GetEnv
which over time diverged. Now the tool's GetEnv is a wrapper around
curl_getenv (libcurl API function which is itself a wrapper around
libcurl's GetEnv).

Furthermore this change fixes a bug in that Windows API
GetEnvironmentVariable() is called instead of C runtime getenv() to get
the environment variable since some changes aren't always visible to the
latter.

Reported-by: Christoph M. Becker

Fixes curl#4774
Closes #xxxx
@jay jay closed this as completed in 9dc350b Feb 12, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Windows Windows-specific
Development

Successfully merging a pull request may close this issue.

3 participants