Skip to content

Commit

Permalink
Use ctime in file digest cache key (bazelbuild#18101)
Browse files Browse the repository at this point in the history
File digests are now additionally keyed by ctime for supported file system implementations. Since Bazel has a non-zero default for `--cache_computed_file_digests`, this may be required for correctness in cases where different files have identical mtime and inode number. For example, this can happen on Linux when files are extracted from a tar file with fixed mtime and are then replaced with `mv`, which preserves inodes.

Since Java (N)IO doesn't have support for reading file ctimes on Windows, a new method backed by a native implementation is added to `WindowsFileOperation`. Adding a call to this function to `stat` uncovered previously silent bugs where Unix-style `PathFragment`s were created on Windows:

1. Bzlmod's `createLocalRepoSpec` did not correctly extract the path from a registry's `file://` URI on Windows.
2. `--package_path` isn't usable with absolute paths on Windows as it splits on `:`. Since the flag is deprecated, this commit fixes the tests rather than the implementation.

Fixes bazelbuild#14723

Closes bazelbuild#18003.

PiperOrigin-RevId: 524297459
Change-Id: I96bfc0210e2f71bf8603c7b7cc5eb06a04048c85

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
  • Loading branch information
keertk and fmeum authored Apr 14, 2023
1 parent 906812c commit 05984b9
Show file tree
Hide file tree
Showing 15 changed files with 284 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/bazel/repository/downloader",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/util:os",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//third_party:gson",
"//third_party:guava",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.gson.FieldNamingPolicy;
import com.google.gson.Gson;
Expand Down Expand Up @@ -175,7 +176,15 @@ private RepoSpec createLocalPathRepoSpec(
path = moduleBase + "/" + path;
if (!PathFragment.isAbsolute(moduleBase)) {
if (uri.getScheme().equals("file")) {
path = uri.getPath() + "/" + path;
if (uri.getPath().isEmpty() || !uri.getPath().startsWith("/")) {
throw new IOException(
String.format(
"Provided non absolute local registry path for module %s: %s",
key, uri.getPath()));
}
// Unix: file:///tmp --> /tmp
// Windows: file:///C:/tmp --> C:/tmp
path = uri.getPath().substring(OS.getCurrent() == OS.WINDOWS ? 1 : 0) + "/" + path;
} else {
throw new IOException(String.format("Provided non local registry for module %s", key));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -719,7 +719,10 @@ private static FileArtifactValue fileArtifactValueFromStat(
}

private void setPathPermissionsIfFile(Path path) throws IOException {
if (path.isFile(Symlinks.NOFOLLOW)) {
FileStatus stat = path.statIfFound(Symlinks.NOFOLLOW);
if (stat != null
&& stat.isFile()
&& stat.getPermissions() != outputPermissions.getPermissionsMode()) {
setPathPermissions(path);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ public long getNodeId() {
return status.getInodeNumber();
}

int getPermissions() {
@Override
public int getPermissions() {
return status.getPermissions();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ private static class CacheKey {
/** File system identifier of the file (typically the inode number). */
private final long nodeId;

/** Last change time of the file. */
private final long changeTime;

/** Last modification time of the file. */
private final long modifiedTime;

Expand All @@ -62,6 +65,7 @@ private static class CacheKey {
public CacheKey(Path path, FileStatus status) throws IOException {
this.path = path.asFragment();
this.nodeId = status.getNodeId();
this.changeTime = status.getLastChangeTime();
this.modifiedTime = status.getLastModifiedTime();
this.size = status.getSize();
}
Expand All @@ -76,6 +80,7 @@ public boolean equals(Object object) {
CacheKey key = (CacheKey) object;
return path.equals(key.path)
&& nodeId == key.nodeId
&& changeTime == key.changeTime
&& modifiedTime == key.modifiedTime
&& size == key.size;
}
Expand All @@ -86,6 +91,7 @@ public int hashCode() {
int result = 17;
result = 31 * result + path.hashCode();
result = 31 * result + Longs.hashCode(nodeId);
result = 31 * result + Longs.hashCode(changeTime);
result = 31 * result + Longs.hashCode(modifiedTime);
result = 31 * result + Longs.hashCode(size);
return result;
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/com/google/devtools/build/lib/vfs/FileStatus.java
Original file line number Diff line number Diff line change
Expand Up @@ -85,4 +85,16 @@ public interface FileStatus {
* ought to cause the node ID of b to change, but appending / modifying b should not.
*/
long getNodeId() throws IOException;

/**
* Returns the file's permissions in POSIX format (e.g. 0755) if possible without performing
* additional IO, otherwise (or if unsupported by the file system) returns -1.
*
* <p>If accurate group and other permissions aren't available, the returned value should attempt
* to mimic a umask of 022 (i.e. read and execute permissions extend to group and other, write
* does not).
*/
default int getPermissions() {
return -1;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,22 @@ public long getNodeId() {
return System.identityHashCode(this);
}

@Override
public final int getPermissions() {
int permissions = 0;
// Emulate the default umask of 022.
if (isReadable) {
permissions |= 0444;
}
if (isWritable) {
permissions |= 0200;
}
if (isExecutable) {
permissions |= 0111;
}
return permissions;
}

@Override
public final InMemoryContentInfo inode() {
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
package com.google.devtools.build.lib.windows;

import com.google.devtools.build.lib.jni.JniLoader;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.AccessDeniedException;

/** File operations on Windows. */
public class WindowsFileOperations {
Expand Down Expand Up @@ -82,6 +84,12 @@ public Status getStatus() {
// IS_SYMLINK_OR_JUNCTION_ERROR = 1;
private static final int IS_SYMLINK_OR_JUNCTION_DOES_NOT_EXIST = 2;

// Keep GET_CHANGE_TIME_* values in sync with src/main/native/windows/file.cc.
private static final int GET_CHANGE_TIME_SUCCESS = 0;
// private static final int GET_CHANGE_TIME_ERROR = 1;
private static final int GET_CHANGE_TIME_DOES_NOT_EXIST = 2;
private static final int GET_CHANGE_TIME_ACCESS_DENIED = 3;

// Keep CREATE_JUNCTION_* values in sync with src/main/native/windows/file.h.
private static final int CREATE_JUNCTION_SUCCESS = 0;
// CREATE_JUNCTION_ERROR = 1;
Expand Down Expand Up @@ -114,6 +122,9 @@ public Status getStatus() {
private static native int nativeIsSymlinkOrJunction(
String path, boolean[] result, String[] error);

private static native int nativeGetChangeTime(
String path, boolean followReparsePoints, long[] result, String[] error);

private static native boolean nativeGetLongPath(String path, String[] result, String[] error);

private static native int nativeCreateJunction(String name, String target, String[] error);
Expand Down Expand Up @@ -143,6 +154,25 @@ public static boolean isSymlinkOrJunction(String path) throws IOException {
throw new IOException(String.format("Cannot tell if '%s' is link: %s", path, error[0]));
}

/** Returns the time at which the file was last changed, including metadata changes. */
public static long getLastChangeTime(String path, boolean followReparsePoints)
throws IOException {
long[] result = new long[] {0};
String[] error = new String[] {null};
switch (nativeGetChangeTime(asLongPath(path), followReparsePoints, result, error)) {
case GET_CHANGE_TIME_SUCCESS:
return result[0];
case GET_CHANGE_TIME_DOES_NOT_EXIST:
throw new FileNotFoundException(path);
case GET_CHANGE_TIME_ACCESS_DENIED:
throw new AccessDeniedException(path);
default:
// This is GET_CHANGE_TIME_ERROR (1). The JNI code puts a custom message in 'error[0]'.
break;
}
throw new IOException(String.format("Cannot get last change time of '%s': %s", path, error[0]));
}

/**
* Returns the long path associated with the input `path`.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOEx
}

final boolean isSymbolicLink = !followSymlinks && fileIsSymbolicLink(file);
final long lastChangeTime =
WindowsFileOperations.getLastChangeTime(getNioPath(path).toString(), followSymlinks);
FileStatus status =
new FileStatus() {
@Override
Expand Down Expand Up @@ -193,15 +195,20 @@ public long getLastModifiedTime() throws IOException {

@Override
public long getLastChangeTime() {
// This is the best we can do with Java NIO...
return attributes.lastModifiedTime().toMillis();
return lastChangeTime;
}

@Override
public long getNodeId() {
// TODO(bazel-team): Consider making use of attributes.fileKey().
return -1;
}

@Override
public int getPermissions() {
// Files on Windows are implicitly readable and executable.
return 0555 | (attributes.isReadOnly() ? 0 : 0200);
}
};

return status;
Expand Down
23 changes: 23 additions & 0 deletions src/main/native/windows/file-jni.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,29 @@ Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeIsSymlink
return static_cast<jint>(result);
}

extern "C" JNIEXPORT jint JNICALL
Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeGetChangeTime(
JNIEnv* env, jclass clazz, jstring path, jboolean follow_reparse_points,
jlongArray result_holder, jobjectArray error_msg_holder) {
std::wstring wpath(bazel::windows::GetJavaWstring(env, path));
std::wstring error;
jlong ctime = 0;
int result =
bazel::windows::GetChangeTime(wpath.c_str(), follow_reparse_points,
reinterpret_cast<int64_t*>(&ctime), &error);
if (result == bazel::windows::GetChangeTimeResult::kSuccess) {
env->SetLongArrayRegion(result_holder, 0, 1, &ctime);
} else {
if (!error.empty() && CanReportError(env, error_msg_holder)) {
ReportLastError(
bazel::windows::MakeErrorMessage(
WSTR(__FILE__), __LINE__, L"nativeGetChangeTime", wpath, error),
env, error_msg_holder);
}
}
return static_cast<jint>(result);
}

extern "C" JNIEXPORT jboolean JNICALL
Java_com_google_devtools_build_lib_windows_WindowsFileOperations_nativeGetLongPath(
JNIEnv* env, jclass clazz, jstring path, jobjectArray result_holder,
Expand Down
49 changes: 49 additions & 0 deletions src/main/native/windows/file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <WinIoCtl.h>
#include <stdint.h> // uint8_t
#include <versionhelpers.h>
#include <winbase.h>
#include <windows.h>

#include <memory>
Expand Down Expand Up @@ -119,6 +120,54 @@ int IsSymlinkOrJunction(const WCHAR* path, bool* result, wstring* error) {
}
}

int GetChangeTime(const WCHAR* path, bool follow_reparse_points,
int64_t* result, wstring* error) {
if (!IsAbsoluteNormalizedWindowsPath(path)) {
if (error) {
*error = MakeErrorMessage(WSTR(__FILE__), __LINE__, L"GetChangeTime",
path, L"expected an absolute Windows path");
}
return GetChangeTimeResult::kError;
}

AutoHandle handle;
DWORD flags = FILE_FLAG_BACKUP_SEMANTICS;
if (!follow_reparse_points) {
flags |= FILE_FLAG_OPEN_REPARSE_POINT;
}
handle = CreateFileW(path, 0,
FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
nullptr, OPEN_EXISTING, flags, nullptr);

if (!handle.IsValid()) {
DWORD err = GetLastError();
if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) {
return GetChangeTimeResult::kDoesNotExist;
} else if (err == ERROR_ACCESS_DENIED) {
return GetChangeTimeResult::kAccessDenied;
}
if (error) {
*error =
MakeErrorMessage(WSTR(__FILE__), __LINE__, L"CreateFileW", path, err);
}
return GetChangeTimeResult::kError;
}

FILE_BASIC_INFO info;
if (!GetFileInformationByHandleEx(handle, FileBasicInfo, (LPVOID)&info,
sizeof(FILE_BASIC_INFO))) {
DWORD err = GetLastError();
if (error) {
*error = MakeErrorMessage(WSTR(__FILE__), __LINE__,
L"GetFileInformationByHandleEx", path, err);
}
return GetChangeTimeResult::kError;
}

*result = info.ChangeTime.QuadPart;
return GetChangeTimeResult::kSuccess;
}

wstring GetLongPath(const WCHAR* path, unique_ptr<WCHAR[]>* result) {
if (!IsAbsoluteNormalizedWindowsPath(path)) {
return MakeErrorMessage(WSTR(__FILE__), __LINE__, L"GetLongPath", path,
Expand Down
17 changes: 17 additions & 0 deletions src/main/native/windows/file.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,16 @@ struct IsSymlinkOrJunctionResult {
};
};

// Keep in sync with j.c.g.devtools.build.lib.windows.WindowsFileOperations
struct GetChangeTimeResult {
enum {
kSuccess = 0,
kError = 1,
kDoesNotExist = 2,
kAccessDenied = 3,
};
};

// Keep in sync with j.c.g.devtools.build.lib.windows.WindowsFileOperations
struct DeletePathResult {
enum {
Expand Down Expand Up @@ -136,6 +146,13 @@ struct ReadSymlinkOrJunctionResult {
// see http://superuser.com/a/343079. In Bazel we only ever create junctions.
int IsSymlinkOrJunction(const WCHAR* path, bool* result, wstring* error);

// Retrieves the FILETIME at which `path` was last changed, including metadata.
//
// `path` should be an absolute, normalized, Windows-style path, with "\\?\"
// prefix if it's longer than MAX_PATH.
int GetChangeTime(const WCHAR* path, bool follow_reparse_points,
int64_t* result, 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
// failure.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,10 @@ public void resettingOutputs() throws Exception {

// Reset this output, which will make the handler stat the file again.
handler.resetOutputs(ImmutableList.of(artifact));
chmodCalls.clear(); // Permit a second chmod call for the artifact.
chmodCalls.clear();
assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(10);
assertThat(chmodCalls).containsExactly(outputPath, 0555);
// The handler should not have chmodded the file as it already has the correct permission.
assertThat(chmodCalls).isEmpty();
}

@Test
Expand Down
Loading

0 comments on commit 05984b9

Please sign in to comment.