Windows: all JNI methods verify their path arg.
Also remove the GetAttributesOfMaybeMissingFile
method because it's based on a false belief,
namely that FindFirstFile returns up-to-date
information. According to this OldNewThing post
that belief was wrong: https://blogs.msdn.microsoft.com/oldnewthing/20111226-00/?p=8813
This PR is a follow-up to https://github.com/bazelbuild/bazel/pull/7176
Closes #7178.
PiperOrigin-RevId: 230705273
diff --git a/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java b/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java
index 741533e..c8243e4 100644
--- a/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java
+++ b/src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java
@@ -104,22 +104,28 @@
String[] result = new String[] {null};
String[] error = new String[] {null};
if (nativeGetLongPath(asLongPath(path), result, error)) {
- return result[0];
+ return removeUncPrefixAndUseSlashes(result[0]);
} else {
throw new IOException(error[0]);
}
}
- /**
- * Returns a Windows-style path suitable to pass to unicode WinAPI functions.
- *
- * <p>Returns an UNC path if `path` is at least `MAX_PATH` long. If it's shorter or is already an
- * UNC path, then this method returns `path` itself.
- */
+ /** Returns a Windows-style path suitable to pass to unicode WinAPI functions. */
static String asLongPath(String path) {
- return path.length() >= MAX_PATH && !path.startsWith("\\\\?\\")
+ return !path.startsWith("\\\\?\\")
? ("\\\\?\\" + path.replace('/', '\\'))
- : path;
+ : path.replace('/', '\\');
+ }
+
+ private static String removeUncPrefixAndUseSlashes(String p) {
+ if (p.length() >= 4
+ && p.charAt(0) == '\\'
+ && (p.charAt(1) == '\\' || p.charAt(1) == '?')
+ && p.charAt(2) == '?'
+ && p.charAt(3) == '\\') {
+ p = p.substring(4);
+ }
+ return p.replace('\\', '/');
}
/**
diff --git a/src/main/native/windows/file-jni.cc b/src/main/native/windows/file-jni.cc
index 0dc4fbd..99b95d8 100644
--- a/src/main/native/windows/file-jni.cc
+++ b/src/main/native/windows/file-jni.cc
@@ -41,13 +41,14 @@
Java_com_google_devtools_build_lib_windows_jni_WindowsFileOperations_nativeIsJunction(
JNIEnv* env, jclass clazz, jstring path, jobjectArray error_msg_holder) {
std::wstring wpath(bazel::windows::GetJavaWstring(env, path));
- int result = bazel::windows::IsJunctionOrDirectorySymlink(wpath.c_str());
+ std::wstring error;
+ int result =
+ bazel::windows::IsJunctionOrDirectorySymlink(wpath.c_str(), &error);
if (result == bazel::windows::IS_JUNCTION_ERROR &&
CanReportError(env, error_msg_holder)) {
- DWORD err_code = GetLastError();
ReportLastError(
bazel::windows::MakeErrorMessage(WSTR(__FILE__), __LINE__,
- L"nativeIsJunction", wpath, err_code),
+ L"nativeIsJunction", wpath, error),
env, error_msg_holder);
}
return result;
@@ -60,11 +61,13 @@
std::unique_ptr<WCHAR[]> result;
std::wstring wpath(bazel::windows::GetJavaWstring(env, path));
std::wstring error(bazel::windows::GetLongPath(wpath.c_str(), &result));
- if (!error.empty() && CanReportError(env, error_msg_holder)) {
- ReportLastError(
- bazel::windows::MakeErrorMessage(WSTR(__FILE__), __LINE__,
- L"nativeGetLongPath", wpath, error),
- env, error_msg_holder);
+ if (!error.empty()) {
+ if (CanReportError(env, error_msg_holder)) {
+ ReportLastError(
+ bazel::windows::MakeErrorMessage(WSTR(__FILE__), __LINE__,
+ L"nativeGetLongPath", wpath, error),
+ env, error_msg_holder);
+ }
return JNI_FALSE;
}
env->SetObjectArrayElement(
diff --git a/src/main/native/windows/file.cc b/src/main/native/windows/file.cc
index fcfae4c..31ef954 100644
--- a/src/main/native/windows/file.cc
+++ b/src/main/native/windows/file.cc
@@ -72,30 +72,23 @@
return wstring(attr_str, 8);
}
-static DWORD GetAttributesOfMaybeMissingFile(const WCHAR* path) {
- // According to a comment in .NET CoreFX [1] (which is the only relevant
- // information we found as of 2018-07-13) GetFileAttributesW may fail with
- // ERROR_ACCESS_DENIED if the file is marked for deletion but not yet
- // actually deleted, but FindFirstFileW should succeed even then.
- //
- // [1]
- // https://github.com/dotnet/corefx/blob/f25eb288a449010574a6e95fe298f3ad880ada1e/src/System.IO.FileSystem/src/System/IO/FileSystem.Windows.cs#L205-L208
- WIN32_FIND_DATAW find_data;
- HANDLE find = FindFirstFileW(path, &find_data);
- if (find == INVALID_HANDLE_VALUE) {
- // The path is deleted and we couldn't create a directory there.
- // Give up.
- return INVALID_FILE_ATTRIBUTES;
+int IsJunctionOrDirectorySymlink(const WCHAR* path, wstring* error) {
+ if (!IsAbsoluteNormalizedWindowsPath(path)) {
+ if (error) {
+ *error = MakeErrorMessage(WSTR(__FILE__), __LINE__,
+ L"IsJunctionOrDirectorySymlink", path,
+ L"expected an absolute Windows path");
+ }
+ return IS_JUNCTION_ERROR;
}
- FindClose(find);
- // The path exists, yet we cannot open it for metadata-reading. Report at
- // least the attributes, then give up.
- return find_data.dwFileAttributes;
-}
-int IsJunctionOrDirectorySymlink(const WCHAR* path) {
DWORD attrs = ::GetFileAttributesW(path);
if (attrs == INVALID_FILE_ATTRIBUTES) {
+ DWORD err = GetLastError();
+ if (error) {
+ *error = MakeErrorMessage(WSTR(__FILE__), __LINE__,
+ L"IsJunctionOrDirectorySymlink", path, err);
+ }
return IS_JUNCTION_ERROR;
} else {
if ((attrs & FILE_ATTRIBUTE_DIRECTORY) &&
@@ -108,14 +101,20 @@
}
wstring GetLongPath(const WCHAR* path, unique_ptr<WCHAR[]>* result) {
- DWORD size = ::GetLongPathNameW(path, NULL, 0);
+ if (!IsAbsoluteNormalizedWindowsPath(path)) {
+ return MakeErrorMessage(WSTR(__FILE__), __LINE__, L"GetLongPath", path,
+ L"expected an absolute Windows path");
+ }
+
+ std::wstring wpath(AddUncPrefixMaybe(path));
+ DWORD size = ::GetLongPathNameW(wpath.c_str(), NULL, 0);
if (size == 0) {
DWORD err_code = GetLastError();
return MakeErrorMessage(WSTR(__FILE__), __LINE__, L"GetLongPathNameW", path,
err_code);
}
result->reset(new WCHAR[size]);
- ::GetLongPathNameW(path, result->get(), size);
+ ::GetLongPathNameW(wpath.c_str(), result->get(), size);
return L"";
}
@@ -142,6 +141,23 @@
int CreateJunction(const wstring& junction_name, const wstring& junction_target,
wstring* error) {
+ if (!IsAbsoluteNormalizedWindowsPath(junction_name)) {
+ if (error) {
+ *error = MakeErrorMessage(
+ WSTR(__FILE__), __LINE__, L"CreateJunction", junction_name,
+ L"expected an absolute Windows path for junction_name");
+ }
+ CreateJunctionResult::kError;
+ }
+ if (!IsAbsoluteNormalizedWindowsPath(junction_target)) {
+ if (error) {
+ *error = MakeErrorMessage(
+ WSTR(__FILE__), __LINE__, L"CreateJunction", junction_target,
+ L"expected an absolute Windows path for junction_target");
+ }
+ CreateJunctionResult::kError;
+ }
+
const wstring target = HasUncPrefix(junction_target.c_str())
? junction_target.substr(4)
: junction_target;
@@ -220,23 +236,11 @@
return CreateJunctionResult::kDisappeared;
}
- wstring err_str = uint32asHexString(err);
// The path seems to exist yet we cannot open it for metadata-reading.
// Report as much information as we have, then give up.
- DWORD attr = GetAttributesOfMaybeMissingFile(name.c_str());
- if (attr == INVALID_FILE_ATTRIBUTES) {
- if (error) {
- *error = MakeErrorMessage(
- WSTR(__FILE__), __LINE__, L"CreateFileW", name,
- wstring(L"err=0x") + err_str + L", invalid attributes");
- }
- } else {
- if (error) {
- *error =
- MakeErrorMessage(WSTR(__FILE__), __LINE__, L"CreateFileW", name,
- wstring(L"err=0x") + err_str + L", attr=0x" +
- uint32asHexString(attr));
- }
+ if (error) {
+ *error = MakeErrorMessage(WSTR(__FILE__), __LINE__, L"CreateFileW",
+ name, err);
}
return CreateJunctionResult::kError;
}
@@ -429,7 +433,7 @@
// The file disappeared, or one of its parent directories disappeared,
// or one of its parent directories is no longer a directory.
return DeletePathResult::kDoesNotExist;
- } else if (err != ERROR_ACCESS_DENIED) {
+ } else {
// Some unknown error occurred.
if (error) {
*error = MakeErrorMessage(WSTR(__FILE__), __LINE__,
@@ -437,14 +441,9 @@
}
return DeletePathResult::kError;
}
-
- attr = GetAttributesOfMaybeMissingFile(wpath);
- if (attr == INVALID_FILE_ATTRIBUTES) {
- // The path is already deleted.
- return DeletePathResult::kDoesNotExist;
- }
}
+ // DeleteFileW failed with access denied, but the path exists.
if (attr & FILE_ATTRIBUTE_DIRECTORY) {
// It's a directory or a junction.
if (!RemoveDirectoryW(wpath)) {
diff --git a/src/main/native/windows/file.h b/src/main/native/windows/file.h
index 05a3274..84cbde3 100644
--- a/src/main/native/windows/file.h
+++ b/src/main/native/windows/file.h
@@ -93,7 +93,7 @@
// created using "mklink" instead of "mklink /d", as such symlinks don't
// behave the same way as directories (e.g. they can't be listed)
// - IS_JUNCTION_ERROR, if `path` doesn't exist or some error occurred
-int IsJunctionOrDirectorySymlink(const WCHAR* path);
+int IsJunctionOrDirectorySymlink(const WCHAR* path, std::wstring* error);
// Computes the long version of `path` if it has any 8dot3 style components.
// Returns the empty string upon success, or a human-readable error message upon
diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsFileOperationsTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsFileOperationsTest.java
index 2f7483a..c5a50d3 100644
--- a/src/test/java/com/google/devtools/build/lib/windows/WindowsFileOperationsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/windows/WindowsFileOperationsTest.java
@@ -90,29 +90,29 @@
testUtil.createJunctions(junctions);
- assertThat(WindowsFileOperations.isJunction(root + "/shrtpath/a")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/shrtpath/b")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/shrtpath/c")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/longlinkpath/a")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/longlinkpath/b")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/longlinkpath/c")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/longli~1/a")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/longli~1/b")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/longli~1/c")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/abbreviated/a")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/abbreviated/b")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/abbreviated/c")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/abbrev~1/a")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/abbrev~1/b")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/abbrev~1/c")).isTrue();
- assertThat(WindowsFileOperations.isJunction(root + "/control/a")).isFalse();
- assertThat(WindowsFileOperations.isJunction(root + "/control/b")).isFalse();
- assertThat(WindowsFileOperations.isJunction(root + "/control/c")).isFalse();
- assertThat(WindowsFileOperations.isJunction(root + "/shrttrgt/file1.txt")).isFalse();
- assertThat(WindowsFileOperations.isJunction(root + "/longtargetpath/file2.txt")).isFalse();
- assertThat(WindowsFileOperations.isJunction(root + "/longta~1/file2.txt")).isFalse();
+ assertThat(WindowsFileOperations.isJunction(root + "\\shrtpath\\a")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\shrtpath\\b")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\shrtpath\\c")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\longlinkpath\\a")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\longlinkpath\\b")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\longlinkpath\\c")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\longli~1\\a")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\longli~1\\b")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\longli~1\\c")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\abbreviated\\a")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\abbreviated\\b")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\abbreviated\\c")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\abbrev~1\\a")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\abbrev~1\\b")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\abbrev~1\\c")).isTrue();
+ assertThat(WindowsFileOperations.isJunction(root + "\\control\\a")).isFalse();
+ assertThat(WindowsFileOperations.isJunction(root + "\\control\\b")).isFalse();
+ assertThat(WindowsFileOperations.isJunction(root + "\\control\\c")).isFalse();
+ assertThat(WindowsFileOperations.isJunction(root + "\\shrttrgt\\file1.txt")).isFalse();
+ assertThat(WindowsFileOperations.isJunction(root + "\\longtargetpath\\file2.txt")).isFalse();
+ assertThat(WindowsFileOperations.isJunction(root + "\\longta~1\\file2.txt")).isFalse();
try {
- WindowsFileOperations.isJunction(root + "/non-existent");
+ WindowsFileOperations.isJunction(root + "\\non-existent");
fail("expected to throw");
} catch (IOException e) {
assertThat(e.getMessage()).contains("nativeIsJunction");
@@ -215,8 +215,8 @@
assertThat(helloFile.exists()).isTrue();
assertThat(new File(longPath).exists()).isTrue();
assertThat(new File(shortPath).exists()).isTrue();
- assertThat(WindowsFileOperations.getLongPath(longPath)).endsWith("will.exist\\helloworld.txt");
- assertThat(WindowsFileOperations.getLongPath(shortPath)).endsWith("will.exist\\helloworld.txt");
+ assertThat(WindowsFileOperations.getLongPath(longPath)).endsWith("will.exist/helloworld.txt");
+ assertThat(WindowsFileOperations.getLongPath(shortPath)).endsWith("will.exist/helloworld.txt");
// Delete the file and the directory, assert that long path resolution fails for them.
assertThat(helloFile.delete()).isTrue();
@@ -243,9 +243,9 @@
.toFile();
assertThat(new File(shortPath).exists()).isTrue();
assertThat(WindowsFileOperations.getLongPath(shortPath))
- .endsWith("will.exist_again\\hellowelt.txt");
+ .endsWith("will.exist_again/hellowelt.txt");
assertThat(WindowsFileOperations.getLongPath(foo + "\\will.exist_again\\hellowelt.txt"))
- .endsWith("will.exist_again\\hellowelt.txt");
+ .endsWith("will.exist_again/hellowelt.txt");
try {
WindowsFileOperations.getLongPath(longPath);
fail("expected to throw");
diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java
index 89303fd..e547074 100644
--- a/src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java
+++ b/src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java
@@ -27,7 +27,6 @@
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Symlinks;
-import com.google.devtools.build.lib.windows.jni.WindowsFileOperations;
import com.google.devtools.build.lib.windows.util.WindowsTestUtil;
import java.io.File;
import java.io.IOException;
@@ -312,7 +311,7 @@
fs.createSymbolicLink(link4, fs.getPath(scratchRoot).getRelative("bar.txt").asFragment());
// Assert that link1 and link2 are true junctions and have the right contents.
for (Path p : ImmutableList.of(link1, link2)) {
- assertThat(WindowsFileOperations.isJunction(p.getPathString())).isTrue();
+ assertThat(WindowsFileSystem.isJunction(new File(p.getPathString()))).isTrue();
assertThat(p.isSymbolicLink()).isTrue();
assertThat(
Iterables.transform(
@@ -327,7 +326,7 @@
}
// Assert that link3 and link4 are copies of files.
for (Path p : ImmutableList.of(link3, link4)) {
- assertThat(WindowsFileOperations.isJunction(p.getPathString())).isFalse();
+ assertThat(WindowsFileSystem.isJunction(new File(p.getPathString()))).isFalse();
assertThat(p.isSymbolicLink()).isFalse();
assertThat(p.isFile()).isTrue();
}
diff --git a/src/test/native/windows/file_test.cc b/src/test/native/windows/file_test.cc
index 5919a37..ca7604d 100644
--- a/src/test/native/windows/file_test.cc
+++ b/src/test/native/windows/file_test.cc
@@ -81,7 +81,8 @@
wstring file1(target + L"\\foo");
EXPECT_TRUE(blaze_util::CreateDummyFile(file1));
- EXPECT_EQ(IS_JUNCTION_NO, IsJunctionOrDirectorySymlink(target.c_str()));
+ EXPECT_EQ(IS_JUNCTION_NO,
+ IsJunctionOrDirectorySymlink(target.c_str(), nullptr));
EXPECT_NE(INVALID_FILE_ATTRIBUTES, ::GetFileAttributesW(file1.c_str()));
wstring name(tmp + L"\\junc_name");
@@ -99,13 +100,13 @@
// Assert creation of the junctions.
ASSERT_EQ(IS_JUNCTION_YES,
- IsJunctionOrDirectorySymlink((name + L"1").c_str()));
+ IsJunctionOrDirectorySymlink((name + L"1").c_str(), nullptr));
ASSERT_EQ(IS_JUNCTION_YES,
- IsJunctionOrDirectorySymlink((name + L"2").c_str()));
+ IsJunctionOrDirectorySymlink((name + L"2").c_str(), nullptr));
ASSERT_EQ(IS_JUNCTION_YES,
- IsJunctionOrDirectorySymlink((name + L"3").c_str()));
+ IsJunctionOrDirectorySymlink((name + L"3").c_str(), nullptr));
ASSERT_EQ(IS_JUNCTION_YES,
- IsJunctionOrDirectorySymlink((name + L"4").c_str()));
+ IsJunctionOrDirectorySymlink((name + L"4").c_str(), nullptr));
// Assert that the file is visible under all junctions.
ASSERT_NE(INVALID_FILE_ATTRIBUTES,
