This is an archive of the discontinued LLVM Phabricator instance.

Fix GetEnv to support environment variables with empty values on windows
ClosedPublic

Authored by bd1976llvm on Aug 7 2017, 7:39 AM.

Details

Summary

An environment variable can be in one of three states:

  1. undefined.
  2. defined with a non-empty value.
  3. defined but with an empty value.

The windows implementation did not support case 3 (it was not handling errors). The Linux implementation is already correct.

Diff Detail

Event Timeline

bd1976llvm created this revision.Aug 7 2017, 7:39 AM
bd1976llvm updated this revision to Diff 110013.Aug 7 2017, 8:52 AM

fixed indentation

ruiu added inline comments.Aug 14 2017, 8:40 AM
lib/Support/Windows/Process.inc
142

Do you have to handle this as a special case?

zturner added inline comments.Aug 14 2017, 2:18 PM
lib/Support/Windows/Process.inc
130–140

Can we change this logic a bit? You only really have to call this function twice (not in a while loop), because if the first call fails it will tell you the exact size you need. So you only need to check the error code once. So I propose writing this like this:

SmallVector<wchar_t, MAX_PATH> Buf;
Buf.reserve(MAX_PATH);
size_t Size = GetEnvironmentVariableW(NameUTF16.data(), Buf.data(), Buf.capacity());
if (Size == 0 && GetLastError() == ERROR_ENVVAR_NOT_FOUND)
  return std::string();
if (Size < Buf.capacity()) {
  Buf.reserve(Size);
  Size = GetEnvironmentVariableW(NameUTF16.data(), Buf.data(), Buf.capacity());
  assert(Size >= Buf.capacity());
}

SmallVector<char, MAX_PATH> Res;
if (windows::UTF16ToUTF8(Buf.data(), Size, Res))
  return None;
return std::string(Res.data(), Res.size());
146

Incidentally, this code won't correctly handle environment variables with embedded nulls. The above code should work though.

@zturner , @ruiu - thanks for the feedback. I will rewrite, test and update the review.

bd1976llvm marked an inline comment as done.Aug 15 2017, 6:28 AM
bd1976llvm added inline comments.
lib/Support/Windows/Process.inc
130–140

No. Consider the case of a race between the two calls. A while loop is safer.

146

I couldn't find a way to get embedded nulls into an environment variable. I don't think this is possible.

Removed unnecessary return.

ruiu added inline comments.Aug 15 2017, 6:57 AM
lib/Support/Windows/Process.inc
130–140

If there's a race condition, clang must be setting an environment variable and reading that variable in a different thread concurrently (because no one can set another processes's environment varaibles). But is it even safe? I mean, I wonder if setenv and getenv thread-safe. It looks to me that you don't want to guard against it but just say that GetEnv and SetEnv are not thread-safe.

bd1976llvm added inline comments.Aug 15 2017, 8:23 AM
lib/Support/Windows/Process.inc
130–140

They are thread safe. The GO people seem to agree with my proposed implementation:

https://github.com/golang/go/issues/9753
https://go-review.googlesource.com/c/4940/4/src/syscall/env_windows.go#22

ruiu accepted this revision.Aug 15 2017, 8:34 AM

LGTM

lib/Support/Windows/Process.inc
135

I believe we usually prefer GetLastError() == ERROR_ENVVAR_NOT_FOUND instead of ERROR_ENVVAR_NOT_FOUND == GetLastError(), just like we did Size == 0 instead of 0 == Size.

This revision is now accepted and ready to land.Aug 15 2017, 8:34 AM

swapped lhs/rhs args in comparison

This revision was automatically updated to reflect the committed changes.
bd1976llvm marked an inline comment as done.