From 4610b52bbbce7a561f3f9446b2070a4cc9d392ea Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 5 Mar 2025 08:00:47 -0800 Subject: [PATCH] [8.5.0] Propagate errors correctly from WindowsFileSystem symlink methods. A bare IOException should not be used in place of FileNotFoundException and NotASymlinkException, which are sometimes used for control flow (to minimize the number of syscalls in a fast path). PiperOrigin-RevId: 733735243 Change-Id: I12b95a61da5ab49a4be15c570df70a5870614bb7 --- .../google/devtools/build/lib/windows/BUILD | 2 + .../lib/windows/WindowsFileOperations.java | 57 +++---------------- .../build/lib/windows/WindowsFileSystem.java | 12 +--- src/main/native/windows/file.cc | 5 +- src/main/native/windows/file.h | 1 - src/main/tools/build-runfiles-windows.cc | 3 - .../windows/WindowsFileOperationsTest.java | 20 +++---- .../lib/windows/WindowsFileSystemTest.java | 40 +++++-------- 8 files changed, 40 insertions(+), 100 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/windows/BUILD b/src/main/java/com/google/devtools/build/lib/windows/BUILD index 96e02b9f50ef3b..5777f506c35851 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/BUILD +++ b/src/main/java/com/google/devtools/build/lib/windows/BUILD @@ -19,6 +19,8 @@ java_11_library( deps = [ ":windows_path_operations", "//src/main/java/com/google/devtools/build/lib/jni", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", ], ) diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java index fea9622b0e7aef..89c7e1484f3a4e 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileOperations.java @@ -15,6 +15,8 @@ package com.google.devtools.build.lib.windows; import com.google.devtools.build.lib.jni.JniLoader; +import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException; +import com.google.devtools.build.lib.vfs.PathFragment; import java.io.FileNotFoundException; import java.io.IOException; import java.nio.file.AccessDeniedException; @@ -49,34 +51,6 @@ private WindowsFileOperations() { // Prevent construction } - /** Result of {@link #readSymlinkOrJunction}. */ - public static class ReadSymlinkOrJunctionResult { - - /** Status code, indicating success or failure. */ - public enum Status { - OK, - NOT_A_LINK, - ERROR - } - - private String result; - private Status status; - - public ReadSymlinkOrJunctionResult(Status s, String r) { - this.status = s; - this.result = r; - } - - /** Result string (junction target) or error message (depending on {@link status}). */ - public String getResult() { - return result; - } - - public Status getStatus() { - return status; - } - } - // Keep IS_SYMLINK_OR_JUNCTION_* values in sync with src/main/native/windows/file.cc. private static final int IS_SYMLINK_OR_JUNCTION_SUCCESS = 0; // IS_SYMLINK_OR_JUNCTION_ERROR = 1; @@ -115,7 +89,6 @@ public Status getStatus() { private static final int READ_SYMLINK_OR_JUNCTION_ACCESS_DENIED = 2; private static final int READ_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST = 3; private static final int READ_SYMLINK_OR_JUNCTION_NOT_A_LINK = 4; - private static final int READ_SYMLINK_OR_JUNCTION_UNKNOWN_LINK_TYPE = 5; private static native int nativeIsSymlinkOrJunction( String path, boolean[] result, String[] error); @@ -140,8 +113,7 @@ public static boolean isSymlinkOrJunction(String path) throws IOException { case IS_SYMLINK_OR_JUNCTION_SUCCESS: return result[0]; case IS_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST: - error[0] = "path does not exist"; - break; + throw new FileNotFoundException(path); default: // This is IS_SYMLINK_OR_JUNCTION_ERROR (1). The JNI code puts a custom message in // 'error[0]'. @@ -225,34 +197,23 @@ public static void createSymlink(String name, String target) throws IOException String.format("Cannot create symlink (name=%s, target=%s): %s", name, target, error[0])); } - public static ReadSymlinkOrJunctionResult readSymlinkOrJunction(String name) { + public static String readSymlinkOrJunction(String name) throws IOException { String[] target = new String[] {null}; String[] error = new String[] {null}; switch (nativeReadSymlinkOrJunction(WindowsPathOperations.asLongPath(name), target, error)) { case READ_SYMLINK_OR_JUNCTION_SUCCESS: - return new ReadSymlinkOrJunctionResult( - ReadSymlinkOrJunctionResult.Status.OK, - WindowsPathOperations.removeUncPrefixAndUseSlashes(target[0])); + return WindowsPathOperations.removeUncPrefixAndUseSlashes(target[0]); case READ_SYMLINK_OR_JUNCTION_ACCESS_DENIED: - error[0] = "access is denied"; - break; + throw new AccessDeniedException(name); case READ_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST: - error[0] = "path does not exist"; - break; + throw new FileNotFoundException(name); case READ_SYMLINK_OR_JUNCTION_NOT_A_LINK: - return new ReadSymlinkOrJunctionResult( - ReadSymlinkOrJunctionResult.Status.NOT_A_LINK, "path is not a link"); - case READ_SYMLINK_OR_JUNCTION_UNKNOWN_LINK_TYPE: - error[0] = "unknown link type"; - break; + throw new NotASymlinkException(PathFragment.create(name)); default: // This is READ_SYMLINK_OR_JUNCTION_ERROR (1). The JNI code puts a custom message in // 'error[0]'. - break; + throw new IOException(String.format("Cannot read link (name=%s): %s", name, error[0])); } - return new ReadSymlinkOrJunctionResult( - ReadSymlinkOrJunctionResult.Status.ERROR, - String.format("Cannot read link (name=%s): %s", name, error[0])); } public static boolean deletePath(String path) throws IOException { diff --git a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java index f697b1143d0000..0ed13f1d06b6a6 100644 --- a/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/windows/WindowsFileSystem.java @@ -103,15 +103,9 @@ protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFrag @Override protected PathFragment readSymbolicLink(PathFragment path) throws IOException { java.nio.file.Path nioPath = getNioPath(path); - WindowsFileOperations.ReadSymlinkOrJunctionResult result = - WindowsFileOperations.readSymlinkOrJunction(nioPath.toString()); - if (result.getStatus() == WindowsFileOperations.ReadSymlinkOrJunctionResult.Status.OK) { - return PathFragment.create(StringEncoding.platformToInternal(result.getResult())); - } - if (result.getStatus() == WindowsFileOperations.ReadSymlinkOrJunctionResult.Status.NOT_A_LINK) { - throw new NotASymlinkException(path); - } - throw new IOException(result.getResult()); + return PathFragment.create( + StringEncoding.platformToInternal( + WindowsFileOperations.readSymlinkOrJunction(nioPath.toString()))); } @Override diff --git a/src/main/native/windows/file.cc b/src/main/native/windows/file.cc index e882f7bef303f4..05007fa8f88b64 100644 --- a/src/main/native/windows/file.cc +++ b/src/main/native/windows/file.cc @@ -607,7 +607,10 @@ int ReadSymlinkOrJunction(const wstring& path, wstring* result, return ReadSymlinkOrJunctionResult::kNotALink; } default: - return ReadSymlinkOrJunctionResult::kUnknownLinkType; + *error = + MakeErrorMessage(WSTR(__FILE__), __LINE__, L"ReadSymlinkOrJunction", + path, L"unsupported link type"); + return ReadSymlinkOrJunctionResult::kError; } } diff --git a/src/main/native/windows/file.h b/src/main/native/windows/file.h index 2850357ffc6088..4e7c6557b655e5 100644 --- a/src/main/native/windows/file.h +++ b/src/main/native/windows/file.h @@ -133,7 +133,6 @@ struct ReadSymlinkOrJunctionResult { kAccessDenied = 2, kDoesNotExist = 3, kNotALink = 4, - kUnknownLinkType = 5, }; }; diff --git a/src/main/tools/build-runfiles-windows.cc b/src/main/tools/build-runfiles-windows.cc index 05c3f265c9463c..90a837498df27e 100644 --- a/src/main/tools/build-runfiles-windows.cc +++ b/src/main/tools/build-runfiles-windows.cc @@ -113,9 +113,6 @@ bool ReadSymlink(const wstring& abs_path, wstring* target, wstring* error) { case bazel::windows::ReadSymlinkOrJunctionResult::kNotALink: *error = L"path is not a link"; break; - case bazel::windows::ReadSymlinkOrJunctionResult::kUnknownLinkType: - *error = L"unknown link type"; - break; default: // This is bazel::windows::ReadSymlinkOrJunctionResult::kError (1). // The JNI code puts a custom message in 'error'. 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 5b628a991a2225..5ff6d9467c551e 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 @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.windows; import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.fail; import com.google.common.collect.ImmutableMap; @@ -22,6 +23,7 @@ import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.windows.util.WindowsTestUtil; import java.io.File; +import java.io.FileNotFoundException; import java.io.IOException; import java.nio.file.Files; import java.util.Arrays; @@ -76,12 +78,9 @@ public void testSymlinkCreation() throws Exception { // Assert deleting the symlink does not remove the target file. assertThat(WindowsFileOperations.deletePath(symlinkFile.toString())).isTrue(); assertThat(helloFile.exists()).isTrue(); - try { - WindowsFileOperations.isSymlinkOrJunction(symlinkFile.toString()); - fail("Expected to throw: Symlink should no longer exist."); - } catch (IOException e) { - assertThat(e).hasMessageThat().contains("path does not exist"); - } + assertThrows( + FileNotFoundException.class, + () -> WindowsFileOperations.isSymlinkOrJunction(symlinkFile.toString())); } @Test @@ -143,12 +142,9 @@ public void testIsJunction() throws Exception { assertThat(WindowsFileOperations.isSymlinkOrJunction(root + "\\longtargetpath\\file2.txt")) .isFalse(); assertThat(WindowsFileOperations.isSymlinkOrJunction(root + "\\longta~1\\file2.txt")).isFalse(); - try { - WindowsFileOperations.isSymlinkOrJunction(root + "\\non-existent"); - fail("expected to throw"); - } catch (IOException e) { - assertThat(e.getMessage()).contains("path does not exist"); - } + assertThrows( + FileNotFoundException.class, + () -> WindowsFileOperations.isSymlinkOrJunction(root + "\\non-existent")); assertThat(Arrays.asList(new File(root + "/shrtpath/a").list())).containsExactly("file1.txt"); assertThat(Arrays.asList(new File(root + "/shrtpath/b").list())).containsExactly("file2.txt"); assertThat(Arrays.asList(new File(root + "/shrtpath/c").list())).containsExactly("file2.txt"); 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 ad5b3fd304aa1d..99d4711c485d7d 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 @@ -15,7 +15,7 @@ package com.google.devtools.build.lib.windows; import static com.google.common.truth.Truth.assertThat; -import static org.junit.Assert.fail; +import static org.junit.Assert.assertThrows; import com.google.common.base.Function; import com.google.common.collect.ImmutableList; @@ -24,12 +24,13 @@ import com.google.devtools.build.lib.testutil.TestSpec; import com.google.devtools.build.lib.util.OS; import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.FileSystem.NotASymlinkException; 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.util.WindowsTestUtil; import java.io.File; -import java.io.IOException; +import java.io.FileNotFoundException; import java.nio.file.Files; import java.util.Arrays; import java.util.HashMap; @@ -166,12 +167,10 @@ public void testIsJunction() throws Exception { .isFalse(); assertThat(WindowsFileSystem.isSymlinkOrJunction(new File(root, "longta~1/file2.txt"))) .isFalse(); - try { - WindowsFileSystem.isSymlinkOrJunction(new File(root, "non-existent")); - fail("expected failure"); - } catch (IOException e) { - assertThat(e.getMessage()).contains("path does not exist"); - } + + assertThrows( + FileNotFoundException.class, + () -> WindowsFileSystem.isSymlinkOrJunction(new File(root, "non-existent"))); assertThat(Arrays.asList(new File(root + "/shrtpath/a").list())).containsExactly("file1.txt"); assertThat(Arrays.asList(new File(root + "/shrtpath/b").list())).containsExactly("file2.txt"); @@ -364,26 +363,15 @@ public void testReadJunction() throws Exception { assertThat(dirPath.isSymbolicLink()).isFalse(); assertThat(juncPath.isSymbolicLink()).isTrue(); - try { - testUtil.createVfsPath(fs, "does-not-exist").readSymbolicLink(); - fail("expected exception"); - } catch (IOException expected) { - assertThat(expected).hasMessageThat().matches(".*path does not exist"); - } + assertThrows( + FileNotFoundException.class, + () -> testUtil.createVfsPath(fs, "does-not-exist").readSymbolicLink()); - try { - testUtil.createVfsPath(fs, "dir\\hello.txt").readSymbolicLink(); - fail("expected exception"); - } catch (IOException expected) { - assertThat(expected).hasMessageThat().matches(".*is not a symlink"); - } + assertThrows( + NotASymlinkException.class, + () -> testUtil.createVfsPath(fs, "dir\\hello.txt").readSymbolicLink()); - try { - dirPath.readSymbolicLink(); - fail("expected exception"); - } catch (IOException expected) { - assertThat(expected).hasMessageThat().matches(".*is not a symlink"); - } + assertThrows(NotASymlinkException.class, () -> dirPath.readSymbolicLink()); assertThat(juncPath.readSymbolicLink()).isEqualTo(dirPath.asFragment()); }