Skip to content

Commit

Permalink
Windows: all JNI methods verify their path arg.
Browse files Browse the repository at this point in the history 
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 #7176
  • Loading branch information
@laszlocsomor
laszlocsomor committed Jan 21, 2019
1 parent f4c4e07 commit 910d871
Show file tree
Hide file tree
Showing 7 changed files with 101 additions and 95 deletions.
21 changes: 12 additions & 9 deletions src/main/java/com/google/devtools/build/lib/windows/jni/WindowsFileOperations.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,22 +104,25 @@ public static String getLongPath(String path) throws IOException {
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('\\', '/');
}

/**
Expand Down
19 changes: 11 additions & 8 deletions src/main/native/windows/file-jni.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,14 @@ extern "C" JNIEXPORT jint JNICALL
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;
Expand All @@ -60,11 +61,13 @@ Java_com_google_devtools_build_lib_windows_jni_WindowsFileOperations_nativeGetLo
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(
Expand Down
87 changes: 43 additions & 44 deletions src/main/native/windows/file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,30 +72,23 @@ static wstring uint32asHexString(uint32_t value) {
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) &&
Expand All @@ -108,14 +101,20 @@ int IsJunctionOrDirectorySymlink(const WCHAR* path) {
}

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"";
}

Expand All @@ -142,6 +141,23 @@ typedef struct _JunctionDescription {

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;
Expand Down Expand Up @@ -220,23 +236,11 @@ int CreateJunction(const wstring& junction_name, const wstring& junction_target,
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;
}
Expand Down Expand Up @@ -429,22 +433,17 @@ int DeletePath(const wstring& path, wstring* error) {
// 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__,
L"GetFileAttributesW", path, err);
}
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)) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/native/windows/file.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ struct CreateJunctionResult {
// 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
Expand Down
52 changes: 26 additions & 26 deletions src/test/java/com/google/devtools/build/lib/windows/WindowsFileOperationsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,29 +90,29 @@ public void testIsJunction() throws Exception {

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");
Expand Down Expand Up @@ -215,8 +215,8 @@ public void testGetLongPath() throws Exception {
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();
Expand All @@ -243,9 +243,9 @@ public void testGetLongPath() throws Exception {
.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");
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/google/devtools/build/lib/windows/WindowsFileSystemTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ public void testCreateSymbolicLink() throws Exception {
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(
Expand All @@ -327,7 +327,7 @@ public String apply(File input) {
}
// 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();
}
Expand Down
11 changes: 6 additions & 5 deletions src/test/native/windows/file_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ TEST_F(WindowsFileOperationsTest, TestCreateJunction) {
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");
Expand All @@ -99,13 +100,13 @@ TEST_F(WindowsFileOperationsTest, TestCreateJunction) {

// 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,
Expand Down

0 comments on commit 910d871

Please sign in to comment.