Repository: bazelbuild/bazel
Issue: Windows, Bazel client: envvars cannot have UNC paths
Labels: type: bug
P2
area-Windows
team-OSS

### Description of the problem / feature request:

Bazel mishandles envvars with UNC paths.

### Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

```
C:\src\tmp>set java
JAVA_HOME=C:\Program Files\Java\jdk1.8.0_162

C:\src\tmp>set JAVA_HOME=\\?\%JAVA_HOME%

C:\src\tmp>set java
JAVA_HOME=\\?\C:\Program Files\Java\jdk1.8.0_162

C:\src\tmp>bazel run :foo
WARNING: Ignoring JAVA_HOME, because it must point to a JDK, not a JRE.
```

### What operating system are you running Bazel on?

Windows 10

### What's the output of `bazel info release`?

release 0.23.1

Issue comments:
/cc @asuffield

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/cpp/util/file_windows.cc b/src/main/cpp/util/file_windows.cc
index 62a19f6f21ac63..9fd150e2851612 100644
--- a/src/main/cpp/util/file_windows.cc
+++ b/src/main/cpp/util/file_windows.cc
@@ -625,17 +625,7 @@ string MakeCanonical(const char* path) {
     return "";
   }
 
-  // Convert the path to lower-case.
-  size_t size =
-      wcslen(long_realpath.get()) - (HasUncPrefix(long_realpath.get()) ? 4 : 0);
-  unique_ptr<WCHAR[]> lcase_realpath(new WCHAR[size + 1]);
-  const WCHAR* p_from = RemoveUncPrefixMaybe(long_realpath.get());
-  WCHAR* p_to = lcase_realpath.get();
-  while (size-- > 0) {
-    *p_to++ = towlower(*p_from++);
-  }
-  *p_to = 0;
-  return string(WstringToCstring(lcase_realpath.get()).get());
+  return string(WstringToCstring(RemoveUncPrefixMaybe(long_realpath.get())).get());
 }
 
 static bool CanReadFileW(const wstring& path) {
@@ -799,12 +789,6 @@ bool MakeDirectories(const string& path, unsigned int mode) {
   return MakeDirectoriesW(wpath, mode);
 }
 
-static inline void ToLowerW(WCHAR* p) {
-  while (*p) {
-    *p++ = towlower(*p);
-  }
-}
-
 std::wstring GetCwdW() {
   static constexpr size_t kBufSmall = MAX_PATH;
   WCHAR buf[kBufSmall];
@@ -816,7 +800,6 @@ std::wstring GetCwdW() {
   }
 
   if (len < kBufSmall) {
-    ToLowerW(buf);
     return std::wstring(buf);
   }
 
@@ -827,7 +810,6 @@ std::wstring GetCwdW() {
     BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
         << "GetCurrentDirectoryW failed (error " << err << ")";
   }
-  ToLowerW(buf_big.get());
   return std::wstring(buf_big.get());
 }
 
diff --git a/src/main/cpp/util/path_platform.h b/src/main/cpp/util/path_platform.h
index 329380f7b9adaa..8d4086a8e6aa55 100644
--- a/src/main/cpp/util/path_platform.h
+++ b/src/main/cpp/util/path_platform.h
@@ -136,7 +136,7 @@ template bool AsAbsoluteWindowsPath<char>(const char *, std::wstring *,
 template bool AsAbsoluteWindowsPath<wchar_t>(const wchar_t *, std::wstring *,
                                              std::string *);
 
-// Same as `AsWindowsPath`, but returns a lowercase 8dot3 style shortened path.
+// Same as `AsWindowsPath`, but returns a 8dot3 style shortened path.
 // Result will never have a UNC prefix, nor a trailing "/" or "\".
 // Works also for non-existent paths; shortens as much of them as it can.
 // Also works for non-existent drives.
diff --git a/src/main/cpp/util/path_windows.cc b/src/main/cpp/util/path_windows.cc
index 0f4227674cdc78..45a932eb44c39e 100644
--- a/src/main/cpp/util/path_windows.cc
+++ b/src/main/cpp/util/path_windows.cc
@@ -65,12 +65,12 @@ std::string ConvertPath(const std::string& path) {
   // The path may not be Windows-style and may not be normalized, so convert it.
   std::string converted_path;
   std::string error;
-  if (!blaze_util::AsWindowsPath(path, &converted_path, &error)) {
+  if (!blaze_util::AsWindowsPath(
+        HasUncPrefix(path.c_str()) ? path.substr(4) : path, &converted_path,
+        &error)) {
     BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR)
         << "ConvertPath(" << path << "): AsWindowsPath failed: " << error;
   }
-  std::transform(converted_path.begin(), converted_path.end(),
-                 converted_path.begin(), ::tolower);
   return converted_path;
 }
 
@@ -83,7 +83,6 @@ std::string MakeAbsolute(const std::string& path) {
         << "MakeAbsolute(" << path
         << "): AsAbsoluteWindowsPath failed: " << error;
   }
-  std::transform(wpath.begin(), wpath.end(), wpath.begin(), ::towlower);
   return std::string(
       WstringToCstring(RemoveUncPrefixMaybe(wpath.c_str())).get());
 }
@@ -115,7 +114,9 @@ std::string MakeAbsoluteAndResolveEnvvars(const std::string& path) {
 }
 
 bool CompareAbsolutePaths(const std::string& a, const std::string& b) {
-  return ConvertPath(a) == ConvertPath(b);
+  std::string aa = ConvertPath(a);
+  std::string bb =  ConvertPath(b);
+  return aa.size() == bb.size() && _stricmp(aa.c_str(), bb.c_str()) == 0;
 }
 
 std::string PathAsJvmFlag(const std::string& path) {
@@ -530,7 +531,6 @@ bool AsShortWindowsPath(const std::string& path, std::string* result,
   }
 
   result->assign(WstringToCstring(wresult.c_str()).get());
-  ToLower(result);
   return true;
 }
 
@@ -570,7 +570,8 @@ static char GetCurrentDrive() {
   std::wstring cwd = GetCwdW();
   wchar_t wdrive = RemoveUncPrefixMaybe(cwd.c_str())[0];
   wchar_t offset = wdrive >= L'A' && wdrive <= L'Z' ? L'A' : L'a';
-  return 'a' + wdrive - offset;
+  bool upper = wdrive >= L'A' && wdrive <= L'Z';
+  return (upper ? 'A' : 'a') + wdrive - offset;
 }
 
 namespace testing {
diff --git a/src/main/tools/build-runfiles-windows.cc b/src/main/tools/build-runfiles-windows.cc
index edb1c916605baf..f371411ab8f455 100644
--- a/src/main/tools/build-runfiles-windows.cc
+++ b/src/main/tools/build-runfiles-windows.cc
@@ -278,9 +278,7 @@ class RunfilesCreator {
 
           if (expected_target == manifest_file_map.end() ||
               expected_target->second.empty()
-              // Both paths are normalized paths in lower case, we can compare
-              // them directly.
-              || target != expected_target->second.c_str() ||
+              || !_wcsicmp(target.c_str(), expected_target->second.c_str()) ||
               blaze_util::IsDirectoryW(target) != is_dir) {
             if (is_dir) {
               RemoveDirectoryOrDie(subpath);


Test patch:
diff --git a/src/test/cpp/util/path_windows_test.cc b/src/test/cpp/util/path_windows_test.cc
index ef29083dfd962c..28c7514597ed02 100644
--- a/src/test/cpp/util/path_windows_test.cc
+++ b/src/test/cpp/util/path_windows_test.cc
@@ -245,13 +245,13 @@ TEST(PathWindowsTest, TestAsWindowsPath) {
   EXPECT_TRUE(error.find("Unix-style") != string::npos);
 
   // Absolute-on-current-drive path gets a drive letter.
-  ASSERT_TRUE(AsWindowsPath("\\foo", &actual, nullptr));
-  ASSERT_EQ(wstring(1, GetCwd()[0]) + L":\\foo", actual);
+  ASSERT_TRUE(AsWindowsPath("\\Foo", &actual, nullptr));
+  ASSERT_EQ(wstring(1, GetCwd()[0]) + L":\\Foo", actual);
 
   // Even for long paths, AsWindowsPath doesn't add a "\\?\" prefix (it's the
   // caller's duty to do so).
-  wstring wlongpath(L"dummy_long_path\\");
-  string longpath("dummy_long_path/");
+  wstring wlongpath(L"DUMMY_long_path\\");
+  string longpath("DUMMY_long_path/");
   while (longpath.size() <= MAX_PATH) {
     wlongpath += wlongpath;
     longpath += longpath;
@@ -299,7 +299,7 @@ TEST(PathWindowsTest, TestAsShortWindowsPath) {
   ASSERT_EQ(string("NUL"), actual);
 
   ASSERT_TRUE(AsShortWindowsPath("C://", &actual, nullptr));
-  ASSERT_EQ(string("c:\\"), actual);
+  ASSERT_EQ(string("C:\\"), actual);
 
   string error;
   ASSERT_FALSE(AsShortWindowsPath("/C//", &actual, &error));
@@ -308,6 +308,9 @@ TEST(PathWindowsTest, TestAsShortWindowsPath) {
   // The A drive usually doesn't exist but AsShortWindowsPath should still work.
   // Here we even have multiple trailing slashes, that should be handled too.
   ASSERT_TRUE(AsShortWindowsPath("A://", &actual, nullptr));
+  ASSERT_EQ(string("A:\\"), actual);
+
+  ASSERT_TRUE(AsShortWindowsPath("a://", &actual, nullptr));
   ASSERT_EQ(string("a:\\"), actual);
 
   // Assert that we can shorten the TEST_TMPDIR.
@@ -327,17 +330,17 @@ TEST(PathWindowsTest, TestAsShortWindowsPath) {
   ASSERT_NE(actual.back(), '/');
   ASSERT_NE(actual.back(), '\\');
 
-  // Assert shortening another long path, and that the result is lowercased.
+  // Assert shortening another long path, and that the result is not lowercased.
   string dirname(JoinPath(short_tmpdir, "LONGpathNAME"));
   ASSERT_EQ(0, mkdir(dirname.c_str()));
   ASSERT_TRUE(PathExists(dirname));
   ASSERT_TRUE(AsShortWindowsPath(dirname, &actual, nullptr));
-  ASSERT_EQ(short_tmpdir + "\\longpa~1", actual);
+  ASSERT_EQ(short_tmpdir + "\\LONGPA~1", actual);
 
-  // Assert shortening non-existent paths.
+  // Assert shortening non-existent paths. Case is preserved.
   ASSERT_TRUE(AsShortWindowsPath(JoinPath(tmpdir, "NonExistent/FOO"), &actual,
                                  nullptr));
-  ASSERT_EQ(short_tmpdir + "\\nonexistent\\foo", actual);
+  ASSERT_EQ(short_tmpdir + "\\NonExistent\\FOO", actual);
 }
 
 TEST(PathWindowsTest, TestMsysRootRetrieval) {
@@ -371,23 +374,22 @@ TEST(PathWindowsTest, IsWindowsDevNullTest) {
 }
 
 TEST(PathWindowsTest, ConvertPathTest) {
-  EXPECT_EQ("c:\\foo", ConvertPath("C:\\FOO"));
+  EXPECT_EQ("C:\\FOO", ConvertPath("C:\\FOO"));
   EXPECT_EQ("c:\\", ConvertPath("c:/"));
-  EXPECT_EQ("c:\\foo\\bar", ConvertPath("c:/../foo\\BAR\\.\\"));
+  EXPECT_EQ("c:\\foo\\BAR", ConvertPath("c:/../foo\\BAR\\.\\"));
 }
 
 TEST(PathWindowsTest, MakeAbsolute) {
-  EXPECT_EQ("c:\\foo\\bar", MakeAbsolute("C:\\foo\\BAR"));
-  EXPECT_EQ("c:\\foo\\bar", MakeAbsolute("C:/foo/bar"));
-  EXPECT_EQ("c:\\foo\\bar", MakeAbsolute("C:\\foo\\bar\\"));
-  EXPECT_EQ("c:\\foo\\bar", MakeAbsolute("C:/foo/bar/"));
-  EXPECT_EQ(blaze_util::AsLower(blaze_util::GetCwd()) + "\\foo",
-            MakeAbsolute("foo"));
-
-  EXPECT_EQ("nul", MakeAbsolute("NUL"));
-  EXPECT_EQ("nul", MakeAbsolute("Nul"));
-  EXPECT_EQ("nul", MakeAbsolute("nul"));
-  EXPECT_EQ("nul", MakeAbsolute("/dev/null"));
+  EXPECT_EQ("C:\\foo\\BAR", MakeAbsolute("C:\\foo\\BAR"));
+  EXPECT_EQ("C:\\foo\\Bar", MakeAbsolute("C:/foo/Bar"));
+  EXPECT_EQ("C:\\foo\\Bar", MakeAbsolute("C:\\foo\\Bar\\"));
+  EXPECT_EQ("C:\\foo\\Bar", MakeAbsolute("C:/foo/Bar/"));
+  EXPECT_EQ(blaze_util::GetCwd() + "\\Foo", MakeAbsolute("Foo"));
+
+  EXPECT_EQ("NUL", MakeAbsolute("NUL"));
+  EXPECT_EQ("NUL", MakeAbsolute("Nul"));
+  EXPECT_EQ("NUL", MakeAbsolute("nul"));
+  EXPECT_EQ("NUL", MakeAbsolute("/dev/null"));
 
   EXPECT_EQ("", MakeAbsolute(""));
 }
@@ -399,16 +401,16 @@ TEST(PathWindowsTest, MakeAbsoluteAndResolveEnvvars_WithTmpdir) {
   char buf[MAX_PATH] = {0};
   DWORD len = ::GetEnvironmentVariableA("TEST_TMPDIR", buf, MAX_PATH);
   const std::string tmpdir = buf;
-  const std::string expected_tmpdir_bar = ConvertPath(tmpdir + "\\bar");
+  const std::string expected_tmpdir_bar = ConvertPath(tmpdir + "\\Bar");
 
   EXPECT_EQ(expected_tmpdir_bar,
-            MakeAbsoluteAndResolveEnvvars("%TEST_TMPDIR%\\bar"));
+            MakeAbsoluteAndResolveEnvvars("%TEST_TMPDIR%\\Bar"));
   EXPECT_EQ(expected_tmpdir_bar,
-            MakeAbsoluteAndResolveEnvvars("%Test_Tmpdir%\\bar"));
+            MakeAbsoluteAndResolveEnvvars("%Test_Tmpdir%\\Bar"));
   EXPECT_EQ(expected_tmpdir_bar,
-            MakeAbsoluteAndResolveEnvvars("%test_tmpdir%\\bar"));
+            MakeAbsoluteAndResolveEnvvars("%test_tmpdir%\\Bar"));
   EXPECT_EQ(expected_tmpdir_bar,
-            MakeAbsoluteAndResolveEnvvars("%test_tmpdir%/bar"));
+            MakeAbsoluteAndResolveEnvvars("%test_tmpdir%/Bar"));
 }
 
 TEST(PathWindowsTest, MakeAbsoluteAndResolveEnvvars_LongPaths) {
@@ -418,4 +420,25 @@ TEST(PathWindowsTest, MakeAbsoluteAndResolveEnvvars_LongPaths) {
   EXPECT_EQ(long_path, MakeAbsoluteAndResolveEnvvars("%long%"));
 }
 
+TEST(PathWindowsTest, CompareAbsolutePathsTest) {
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "c:/foo"));
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "c:/foo/"));
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "C:\\FOO"));
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "C:\\FOO\\"));
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "\\\\?\\C:\\FOO"));
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "\\\\?\\C:\\FOO\\"));
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "C:\\BAR\\..\\FOO"));
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "C:\\BAR\\..\\FOO\\"));
+  EXPECT_TRUE(CompareAbsolutePaths("c:\\foo", "C:\\BAR\\..\\FOO\\"));
+
+  EXPECT_TRUE(CompareAbsolutePaths("NUL", "Nul"));
+  EXPECT_TRUE(CompareAbsolutePaths("NUL", "nul"));
+  EXPECT_TRUE(CompareAbsolutePaths("NUL", "/dev/null"));
+
+  EXPECT_FALSE(CompareAbsolutePaths("c:\\foo", "c:\\foo."));
+  EXPECT_FALSE(CompareAbsolutePaths("c:\\foo", "c:\\foo "));
+  EXPECT_FALSE(CompareAbsolutePaths("c:\\foo", "\\\\?\\c:\\foo."));
+  EXPECT_FALSE(CompareAbsolutePaths("c:\\foo", "c:\\bar"));
+}
+
 }  // namespace blaze_util