From 5d955f515fc3211cccaf0e999c1edf439cf22f21 Mon Sep 17 00:00:00 2001 From: janakr Date: Thu, 24 Jun 2021 15:33:25 -0700 Subject: [PATCH] Distinguish between I/O exceptions caused by user decisions versus environmental issues: symlink cycles are the user's fault, and should have a different exit code from generic I/O exceptions. PiperOrigin-RevId: 381351065 --- .../google/devtools/build/lib/packages/BUILD | 1 + .../lib/packages/StarlarkNativeModule.java | 12 ++++++- .../lib/unix/InvalidArgumentIOException.java | 27 ++++++++++++++++ .../build/lib/unix/UnixFileSystem.java | 5 ++- .../com/google/devtools/build/lib/vfs/BUILD | 1 + .../lib/vfs/FileSymlinkLoopException.java | 32 +++++++++++++++++++ .../devtools/build/lib/vfs/FileSystem.java | 9 ++++-- src/main/native/unix_jni.cc | 10 ++++-- src/main/protobuf/failure_details.proto | 2 +- .../google/devtools/build/lib/buildtool/BUILD | 2 ++ ...tomRealFilesystemBuildIntegrationTest.java | 18 ++++++----- .../lib/buildtool/DanglingSymlinkTest.java | 29 +++++++++++++++++ 12 files changed, 131 insertions(+), 17 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/unix/InvalidArgumentIOException.java create mode 100644 src/main/java/com/google/devtools/build/lib/vfs/FileSymlinkLoopException.java diff --git a/src/main/java/com/google/devtools/build/lib/packages/BUILD b/src/main/java/com/google/devtools/build/lib/packages/BUILD index 0ec353b096726a..a98893183eacc0 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BUILD +++ b/src/main/java/com/google/devtools/build/lib/packages/BUILD @@ -37,6 +37,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/collect/nestedset", "//src/main/java/com/google/devtools/build/lib/concurrent", "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/io:file_symlink_exception", "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/skyframe:detailed_exceptions", diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java index dc031941b08bdc..bc1e62b3da12ca 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.cmdline.LabelValidator; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.io.FileSymlinkException; import com.google.devtools.build.lib.packages.Globber.BadGlobException; import com.google.devtools.build.lib.packages.PackageFactory.PackageContext; import com.google.devtools.build.lib.packages.RuleClass.Builder.ThirdPartyLicenseExistencePolicy; @@ -114,7 +115,16 @@ public Sequence glob( excludes.isEmpty() ? "" : " - [" + Joiner.on(", ").join(excludes) + "]", e.getMessage()); Location loc = thread.getCallerLocation(); - Event error = Package.error(loc, errorMessage, Code.GLOB_IO_EXCEPTION); + Event error = + Package.error( + loc, + errorMessage, + // If there are other IOExceptions that can result from user error, they should be + // tested for here. Currently FileNotFoundException is not one of those, because globs + // only encounter that error in the presence of an inconsistent filesystem. + e instanceof FileSymlinkException + ? Code.EVAL_GLOBS_SYMLINK_ERROR + : Code.GLOB_IO_EXCEPTION); context.eventHandler.handle(error); context.pkgBuilder.setIOException(e, errorMessage, error.getProperty(DetailedExitCode.class)); matches = ImmutableList.of(); diff --git a/src/main/java/com/google/devtools/build/lib/unix/InvalidArgumentIOException.java b/src/main/java/com/google/devtools/build/lib/unix/InvalidArgumentIOException.java new file mode 100644 index 00000000000000..4f0454ebe8aa0c --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/unix/InvalidArgumentIOException.java @@ -0,0 +1,27 @@ +// Copyright 2021 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.unix; + +import java.io.IOException; + +/** + * {@link IOException} indicating that the argument to an operation was invalid, like trying to read + * a normal file as a symbolic link. Instantiated only by JNI. + */ +public class InvalidArgumentIOException extends IOException { + public InvalidArgumentIOException(String message) { + super(message); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java index b0e5b0b7450a8a..397b390d63fd7c 100644 --- a/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/unix/UnixFileSystem.java @@ -344,9 +344,8 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException { long startTime = Profiler.nanoTimeMaybe(); try { return PathFragment.create(NativePosixFiles.readlink(name)); - } catch (IOException e) { - // EINVAL => not a symbolic link. Anything else is a real error. - throw e.getMessage().endsWith("(Invalid argument)") ? new NotASymlinkException(path) : e; + } catch (InvalidArgumentIOException e) { + throw new NotASymlinkException(path, e); } finally { profiler.logSimpleTask(startTime, ProfilerTask.VFS_READLINK, name); } diff --git a/src/main/java/com/google/devtools/build/lib/vfs/BUILD b/src/main/java/com/google/devtools/build/lib/vfs/BUILD index 8af5688bd11037..bcd1791fe5c213 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/BUILD +++ b/src/main/java/com/google/devtools/build/lib/vfs/BUILD @@ -51,6 +51,7 @@ java_library( ":pathfragment", "//src/main/java/com/google/devtools/build/lib/clock", "//src/main/java/com/google/devtools/build/lib/concurrent", + "//src/main/java/com/google/devtools/build/lib/io:file_symlink_exception", "//src/main/java/com/google/devtools/build/lib/profiler", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec", "//src/main/java/com/google/devtools/build/lib/util:filetype", diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSymlinkLoopException.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSymlinkLoopException.java new file mode 100644 index 00000000000000..4026e39441dbc0 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSymlinkLoopException.java @@ -0,0 +1,32 @@ +// Copyright 2021 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.devtools.build.lib.vfs; + +import com.google.devtools.build.lib.io.FileSymlinkException; + +final class FileSymlinkLoopException extends FileSymlinkException { + FileSymlinkLoopException(String message) { + super(message); + } + + FileSymlinkLoopException(PathFragment pathFragment) { + this(pathFragment.getPathString() + " (Too many levels of symbolic links)"); + } + + @Override + public String getUserFriendlyMessage() { + return getMessage(); + } +} diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java index 4acb2402179581..51c000664ae23e 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystem.java @@ -52,7 +52,11 @@ public DigestHashFunction getDigestFunction() { */ protected static final class NotASymlinkException extends IOException { public NotASymlinkException(PathFragment path) { - super(path + " is not a symlink"); + super(path.getPathString() + " is not a symlink"); + } + + public NotASymlinkException(PathFragment path, Throwable cause) { + super(path.getPathString() + " is not a symlink", cause); } } @@ -354,7 +358,7 @@ protected final PathFragment appendSegment(PathFragment dir, String child, int m } if (maxLinks-- == 0) { - throw new IOException(naive + " (Too many levels of symbolic links)"); + throw new FileSymlinkLoopException(naive); } if (linkTarget.isAbsolute()) { dir = PathFragment.createAlreadyNormalized(linkTarget.getDriveStr()); @@ -774,4 +778,5 @@ protected abstract void createFSDependentHardLink( * implement this in order to warm the filesystem's internal caches. */ protected void prefetchPackageAsync(PathFragment path, int maxDirs) {} + } diff --git a/src/main/native/unix_jni.cc b/src/main/native/unix_jni.cc index 1e88d3a093c005..bc394c6eebc5ab 100644 --- a/src/main/native/unix_jni.cc +++ b/src/main/native/unix_jni.cc @@ -81,10 +81,17 @@ void PostException(JNIEnv *env, int error_number, const std::string& message) { // (aka EOPNOTSUPP) exception_classname = "java/lang/UnsupportedOperationException"; break; + case EINVAL: // Invalid argument + exception_classname = + "com/google/devtools/build/lib/unix/InvalidArgumentIOException"; + break; + case ELOOP: // Too many symbolic links encountered + exception_classname = + "com/google/devtools/build/lib/vfs/FileSymlinkLoopException"; + break; case EBADF: // Bad file number or descriptor already closed. case ENAMETOOLONG: // File name too long case ENODATA: // No data available - case EINVAL: // Invalid argument #if defined(EMULTIHOP) case EMULTIHOP: // Multihop attempted #endif @@ -98,7 +105,6 @@ void PostException(JNIEnv *env, int error_number, const std::string& message) { case EROFS: // Read-only file system case EEXIST: // File exists case EMLINK: // Too many links - case ELOOP: // Too many symbolic links encountered case EISDIR: // Is a directory case ENOTDIR: // Not a directory case ENOTEMPTY: // Directory not empty diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index 94fcdccc5fbb02..996991ef61ac4a 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -1186,7 +1186,7 @@ message PackageLoading { PACKAGE_MISSING = 11 [(metadata) = { exit_code: 1 }]; TARGET_MISSING = 12 [(metadata) = { exit_code: 1 }]; NO_SUCH_THING = 13 [(metadata) = { exit_code: 1 }]; - GLOB_IO_EXCEPTION = 14 [(metadata) = { exit_code: 1 }]; + GLOB_IO_EXCEPTION = 14 [(metadata) = { exit_code: 36 }]; DUPLICATE_LABEL = 15 [(metadata) = { exit_code: 1 }]; INVALID_PACKAGE_SPECIFICATION = 16 [(metadata) = { exit_code: 1 }]; SYNTAX_ERROR = 17 [(metadata) = { exit_code: 1 }]; diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD index bb61f9f7c47221..1d7148a7b20871 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/BUILD +++ b/src/test/java/com/google/devtools/build/lib/buildtool/BUILD @@ -180,8 +180,10 @@ java_test( ], deps = [ "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/main/protobuf:failure_details_java_proto", "//src/test/java/com/google/devtools/build/lib/buildtool/util", "//src/test/java/com/google/devtools/build/lib/packages:testutil", "//third_party:junit4", diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/CustomRealFilesystemBuildIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/CustomRealFilesystemBuildIntegrationTest.java index 6a8322186193b9..79dcecf005a46c 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/CustomRealFilesystemBuildIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/CustomRealFilesystemBuildIntegrationTest.java @@ -34,7 +34,6 @@ import com.google.devtools.build.lib.vfs.Dirent; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystem; -import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; @@ -97,15 +96,18 @@ public void testIOExceptionMidLevel() throws Exception { } @Test - public void globDanglingSymlink() throws Exception { - Path packageDirPath = write("foo/BUILD", "exports_files(glob(['*.txt']))").getParentDirectory(); - write("foo/existing.txt"); - Path badSymlink = packageDirPath.getChild("bad.txt"); - FileSystemUtils.ensureSymbolicLink(badSymlink, "nope"); - customFileSystem.alwaysError(badSymlink); + public void globIOException() throws Exception { + Path fooBuild = write("foo/BUILD", "exports_files(glob(['**/*.txt']))").getParentDirectory(); + Path badDir = fooBuild.getChild("bad"); + assertThat(badDir.createDirectory()).isTrue(); + customFileSystem.alwaysError(badDir); TargetParsingException e = assertThrows(TargetParsingException.class, () -> buildTarget("//foo:all")); - assertThat(e).hasMessageThat().contains("no such package 'foo': error globbing [*.txt]: nope"); + assertThat(e) + .hasMessageThat() + .contains("no such package 'foo': error globbing [**/*.txt]: nope"); + assertThat(e.getDetailedExitCode().getFailureDetail().getPackageLoading().getCode()) + .isEqualTo(FailureDetails.PackageLoading.Code.GLOB_IO_EXCEPTION); } /** diff --git a/src/test/java/com/google/devtools/build/lib/buildtool/DanglingSymlinkTest.java b/src/test/java/com/google/devtools/build/lib/buildtool/DanglingSymlinkTest.java index 23908371643992..c329b3750764f6 100644 --- a/src/test/java/com/google/devtools/build/lib/buildtool/DanglingSymlinkTest.java +++ b/src/test/java/com/google/devtools/build/lib/buildtool/DanglingSymlinkTest.java @@ -18,7 +18,10 @@ import com.google.devtools.build.lib.actions.BuildFailedException; import com.google.devtools.build.lib.buildtool.util.GoogleBuildIntegrationTestCase; +import com.google.devtools.build.lib.cmdline.TargetParsingException; import com.google.devtools.build.lib.packages.util.MockGenruleSupport; +import com.google.devtools.build.lib.server.FailureDetails; +import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import org.junit.Before; @@ -112,4 +115,30 @@ public void testDanglingSymlinkMidLevel() throws Exception { assertThrows(BuildFailedException.class, () -> buildTarget("//foo:top")); events.assertContainsError("Symlinking //foo:foo failed: missing input file '//foo:foo.sh'"); } + + @Test + public void globDanglingSymlink() throws Exception { + Path packageDirPath = write("foo/BUILD", "exports_files(glob(['*.txt']))").getParentDirectory(); + write("foo/existing.txt"); + Path badSymlink = packageDirPath.getChild("bad.txt"); + FileSystemUtils.ensureSymbolicLink(badSymlink, "nope"); + // Successful build: dangling symlinks in glob are ignored. + buildTarget("//foo:all"); + } + + @Test + public void globSymlinkCycle() throws Exception { + Path fooBuildFile = write("foo/BUILD", "sh_library(name = 'foo', srcs = glob(['*.sh']))"); + fooBuildFile + .getParentDirectory() + .getChild("foo.sh") + .createSymbolicLink(PathFragment.create("foo.sh")); + TargetParsingException e = + assertThrows(TargetParsingException.class, () -> buildTarget("//foo:foo")); + assertThat(e.getDetailedExitCode().getFailureDetail().getPackageLoading().getCode()) + .isEqualTo(FailureDetails.PackageLoading.Code.EVAL_GLOBS_SYMLINK_ERROR); + } + + @Test + public void globMissingFile() throws Exception {} }