From 55c296a49296896a2461aba3dc84621ed50f58c2 Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Wed, 7 Feb 2024 16:37:08 +0100 Subject: [PATCH 1/8] Fix typos --- pkg/filesystem/local_directory_windows.go | 2 +- pkg/util/non_empty_stack.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/filesystem/local_directory_windows.go b/pkg/filesystem/local_directory_windows.go index 00358242..39ce041f 100644 --- a/pkg/filesystem/local_directory_windows.go +++ b/pkg/filesystem/local_directory_windows.go @@ -869,7 +869,7 @@ func createNTFSHardlink(oldHandle windows.Handle, oldName string, newHandle wind func renameHelper(sourceHandle, newHandle windows.Handle, newName string) (areSame bool, err error) { // We want to know a few things before renaming: // 1. Are source and target hard links to the same file? If so, noop. - // 2. If target exists and wither source or target is a directory, don't overwrite and report error. + // 2. If target exists and whether source or target is a directory, don't overwrite and report error. // 3. If neither is the case, move and, if necessary, replace. var targetHandle windows.Handle err = ntCreateFile(&targetHandle, windows.FILE_READ_ATTRIBUTES, newHandle, newName, windows.FILE_OPEN, diff --git a/pkg/util/non_empty_stack.go b/pkg/util/non_empty_stack.go index ff1a8fbc..0cc7f115 100644 --- a/pkg/util/non_empty_stack.go +++ b/pkg/util/non_empty_stack.go @@ -34,7 +34,7 @@ func (cw *NonEmptyStack[T]) Push(d T) { // PopSingle removes the last pushed element from the stack. The return // value indicates whether an element was popped successfully. It is not -// possible to push the final element off the stack. +// possible to pop the final element off the stack. func (cw *NonEmptyStack[T]) PopSingle() (T, bool) { if len(cw.stack) == 1 { var zero T From e2991237fe52aba376d1b94cee3c2015ec6d43f0 Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Wed, 7 Feb 2024 16:37:08 +0100 Subject: [PATCH 2/8] Improve errors --- pkg/filesystem/BUILD.bazel | 1 + pkg/filesystem/local_directory_windows.go | 5 +++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/filesystem/BUILD.bazel b/pkg/filesystem/BUILD.bazel index 868897e7..06bf62ba 100644 --- a/pkg/filesystem/BUILD.bazel +++ b/pkg/filesystem/BUILD.bazel @@ -51,6 +51,7 @@ go_library( ], "@io_bazel_rules_go//go/platform:windows": [ "//pkg/filesystem/windowsext", + "//pkg/util", "@org_golang_google_grpc//codes", "@org_golang_google_grpc//status", "@org_golang_x_sys//windows", diff --git a/pkg/filesystem/local_directory_windows.go b/pkg/filesystem/local_directory_windows.go index 39ce041f..d0ab1c5e 100644 --- a/pkg/filesystem/local_directory_windows.go +++ b/pkg/filesystem/local_directory_windows.go @@ -16,6 +16,7 @@ import ( "github.com/buildbarn/bb-storage/pkg/filesystem/path" "github.com/buildbarn/bb-storage/pkg/filesystem/windowsext" + "github.com/buildbarn/bb-storage/pkg/util" "golang.org/x/sys/windows" "google.golang.org/grpc/codes" @@ -606,7 +607,7 @@ func (d *localDirectory) RemoveAllChildren() error { err = subdirectory.RemoveAllChildren() subdirectory.Close() if err != nil { - return err + return util.StatusWrapf(err, "%s: ", name) } } err = d.Remove(component) @@ -624,7 +625,7 @@ func (d *localDirectory) RemoveAll(name path.Component) error { err := subdirectory.RemoveAllChildren() subdirectory.Close() if err != nil { - return err + return util.StatusWrapf(err, "%s: ", name) } return d.Remove(name) } else if err == syscall.ENOTDIR { From 9de9f106287adea782294b2456ee8440c7816669 Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Wed, 7 Feb 2024 16:37:08 +0100 Subject: [PATCH 3/8] Update 'aspect_rules_js' This is required to build the otel stylesheet on Windows. --- WORKSPACE | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 8e13aa69..07b58bb5 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -163,9 +163,9 @@ http_archive( http_archive( name = "aspect_rules_js", - sha256 = "00e7b97b696af63812df0ca9e9dbd18579f3edd3ab9a56f227238b8405e4051c", - strip_prefix = "rules_js-1.23.0", - url = "https://github.com/aspect-build/rules_js/releases/download/v1.23.0/rules_js-v1.23.0.tar.gz", + sha256 = "a949d56fed8fa0a8dd82a0a660acc949253a05b2b0c52a07e4034e27f11218f6", + strip_prefix = "rules_js-1.33.1", + url = "https://github.com/aspect-build/rules_js/releases/download/v1.33.1/rules_js-v1.33.1.tar.gz", ) load("@aspect_rules_js//js:repositories.bzl", "rules_js_dependencies") From 0beee53fa33fad41988931c8c9633166b2eb8693 Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Wed, 7 Feb 2024 16:37:08 +0100 Subject: [PATCH 4/8] Explicitly request query label output To work with users who prefer a different output in their personal rc file. Using '--nohome_rc' is not good either, if they want to change the output base this query would duplicate a lot of data. --- .github/workflows/master.yaml | 2 +- .github/workflows/pull-requests.yaml | 2 +- tools/github_workflows/workflows_template.libsonnet | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/master.yaml b/.github/workflows/master.yaml index cc9c8056..cbb00586 100644 --- a/.github/workflows/master.yaml +++ b/.github/workflows/master.yaml @@ -41,7 +41,7 @@ }, { "name": "Protobuf generation", - "run": "find . bazel-bin/pkg/proto -name '*.pb.go' -delete || true\nbazel build $(bazel query 'kind(\"go_proto_library\", //...)')\nfind bazel-bin/pkg/proto -name '*.pb.go' | while read f; do\n cat $f > $(echo $f | sed -e 's|.*/pkg/proto/|pkg/proto/|')\ndone\n" + "run": "find . bazel-bin/pkg/proto -name '*.pb.go' -delete || true\nbazel build $(bazel query --output=label 'kind(\"go_proto_library\", //...)')\nfind bazel-bin/pkg/proto -name '*.pb.go' | while read f; do\n cat $f > $(echo $f | sed -e 's|.*/pkg/proto/|pkg/proto/|')\ndone\n" }, { "name": "Test style conformance", diff --git a/.github/workflows/pull-requests.yaml b/.github/workflows/pull-requests.yaml index c90ef35b..cd400a59 100644 --- a/.github/workflows/pull-requests.yaml +++ b/.github/workflows/pull-requests.yaml @@ -41,7 +41,7 @@ }, { "name": "Protobuf generation", - "run": "find . bazel-bin/pkg/proto -name '*.pb.go' -delete || true\nbazel build $(bazel query 'kind(\"go_proto_library\", //...)')\nfind bazel-bin/pkg/proto -name '*.pb.go' | while read f; do\n cat $f > $(echo $f | sed -e 's|.*/pkg/proto/|pkg/proto/|')\ndone\n" + "run": "find . bazel-bin/pkg/proto -name '*.pb.go' -delete || true\nbazel build $(bazel query --output=label 'kind(\"go_proto_library\", //...)')\nfind bazel-bin/pkg/proto -name '*.pb.go' | while read f; do\n cat $f > $(echo $f | sed -e 's|.*/pkg/proto/|pkg/proto/|')\ndone\n" }, { "name": "Test style conformance", diff --git a/tools/github_workflows/workflows_template.libsonnet b/tools/github_workflows/workflows_template.libsonnet index 8efcd440..6a2d8259 100644 --- a/tools/github_workflows/workflows_template.libsonnet +++ b/tools/github_workflows/workflows_template.libsonnet @@ -96,7 +96,7 @@ name: 'Protobuf generation', run: ||| find . bazel-bin/pkg/proto -name '*.pb.go' -delete || true - bazel build $(bazel query 'kind("go_proto_library", //...)') + bazel build $(bazel query --output=label 'kind("go_proto_library", //...)') find bazel-bin/pkg/proto -name '*.pb.go' | while read f; do cat $f > $(echo $f | sed -e 's|.*/pkg/proto/|pkg/proto/|') done From ec981ba361901ee5b563e6db955a0414449db627 Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Wed, 7 Feb 2024 16:37:08 +0100 Subject: [PATCH 5/8] Improve absolute path handling on Windows In the presence of forward-slash paths with drive letters. They can come from environment variable expansion in the proto configuration. --- pkg/filesystem/local_directory_windows.go | 30 ++++++++++++++++++++--- pkg/filesystem/path/resolve.go | 7 ++++++ 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/pkg/filesystem/local_directory_windows.go b/pkg/filesystem/local_directory_windows.go index d0ab1c5e..44241dee 100644 --- a/pkg/filesystem/local_directory_windows.go +++ b/pkg/filesystem/local_directory_windows.go @@ -108,10 +108,34 @@ func newLocalDirectory(absPath string, openReparsePoint bool) (DirectoryCloser, return newLocalDirectoryFromHandle(handle) } +// Convert forward-slash drive-letter paths to absolute drive-letter paths. +// This can come from how build directories are configured for bb-worker on Windows. +// Where a combination of 'git-bash' and environment variable expansion in jsonnet can create paths +// that the `filepath` does not detect as absolute. +// +// Example: /C:/... +// Should be C:/... +func CanonicalizeAbsolutePath(path string) string { + if len(path) >= 4 && path[0] == '/' && path[2] == ':' && path[3] == '/' { + path = path[1:] + } + + return path +} + func NewLocalDirectory(path string) (DirectoryCloser, error) { - absPath, err := filepath.Abs(path) - if err != nil { - return nil, err + var absPath string + var err error + + path = CanonicalizeAbsolutePath(path) + + if filepath.IsAbs(path) { + absPath = filepath.FromSlash(path) + } else { + absPath, err = filepath.Abs(path) + if err != nil { + return nil, err + } } absPath = "\\??\\" + absPath return newLocalDirectory(absPath, true) diff --git a/pkg/filesystem/path/resolve.go b/pkg/filesystem/path/resolve.go index 80781574..4d14398e 100644 --- a/pkg/filesystem/path/resolve.go +++ b/pkg/filesystem/path/resolve.go @@ -1,6 +1,7 @@ package path import ( + "runtime" "strings" "google.golang.org/grpc/codes" @@ -38,6 +39,12 @@ func (rs *resolverState) push(scopeWalker ScopeWalker, path string) error { path = stripOneOrMoreSlashes(path) absolute = true } + if runtime.GOOS == "windows" { + // filepath.IsAbs + if len(path) > 3 && path[1] == ':' && path[2] == '/' { + absolute = true + } + } // Push the path without any leading slashes onto the stack, so // that its components may be processed. Apply them against the From fa2041e9626b0ede150d41ec32bacc003d0ddaa2 Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Wed, 7 Feb 2024 16:37:08 +0100 Subject: [PATCH 6/8] Windows: Fix 'readdirnames': fetch all the directory entries. The 'ERROR_MORE_DATA' error is never raised so this continues parsing until the bookend 'ERROR_NO_MORE_DATA' is raised instead. --- pkg/filesystem/local_directory_windows.go | 73 +++++++++++------------ 1 file changed, 36 insertions(+), 37 deletions(-) diff --git a/pkg/filesystem/local_directory_windows.go b/pkg/filesystem/local_directory_windows.go index 44241dee..85553eed 100644 --- a/pkg/filesystem/local_directory_windows.go +++ b/pkg/filesystem/local_directory_windows.go @@ -460,49 +460,48 @@ func (d *localDirectory) Mknod(name path.Component, perm os.FileMode, deviceNumb } func readdirnames(handle windows.Handle) ([]string, error) { - outBufferSize := uint32(512) - outBuffer := make([]byte, outBufferSize) - firstIteration := true + outBufferSize := uint32(256) + names := make([]string, 0) + for { - err := windows.GetFileInformationByHandleEx(handle, windows.FileFullDirectoryInfo, - &outBuffer[0], outBufferSize) - if err == nil { - break - } - if err.(syscall.Errno) == windows.ERROR_NO_MORE_FILES { - if firstIteration { - return []string{}, nil + outBufferSize *= 2 + outBuffer := make([]byte, outBufferSize) + + err := windows.GetFileInformationByHandleEx(handle, windows.FileFullDirectoryInfo, &outBuffer[0], outBufferSize) + if err != nil { + if err.(syscall.Errno) == windows.ERROR_NO_MORE_FILES { + break + } + // NB: We have never seen `ERROR_MORE_DATA` during development, + // it seems it is never raised. But according to the docs is should be set + // and we should proceed with the happy path. + if err.(syscall.Errno) != windows.ERROR_MORE_DATA { + return []string{}, err } - break - } - if err.(syscall.Errno) == windows.ERROR_MORE_DATA { - outBufferSize *= 2 - outBuffer = make([]byte, outBufferSize) - } else { - return nil, err - } - firstIteration = false - } - names := make([]string, 0) - offset := ^(uint32(0)) - dirInfoPtr := (*windowsext.FILE_FULL_DIR_INFO)(unsafe.Pointer(&outBuffer[0])) - for offset != 0 { - offset = dirInfoPtr.NextEntryOffset - fileNameLen := int(dirInfoPtr.FileNameLength) / 2 - fileNameUTF16 := make([]uint16, fileNameLen) - targetPtr := unsafe.Pointer(&dirInfoPtr.FileName[0]) - for i := 0; i < fileNameLen; i++ { - fileNameUTF16[i] = *(*uint16)(targetPtr) - targetPtr = unsafe.Pointer(uintptr(targetPtr) + uintptr(2)) } - dirInfoPtr = (*windowsext.FILE_FULL_DIR_INFO)(unsafe.Pointer(uintptr(unsafe.Pointer(dirInfoPtr)) + uintptr(offset))) - fileName := windows.UTF16ToString(fileNameUTF16) - if fileName == "." || fileName == ".." { - continue + offset := ^(uint32(0)) + dirInfoPtr := (*windowsext.FILE_FULL_DIR_INFO)(unsafe.Pointer(&outBuffer[0])) + for offset != 0 { + offset = dirInfoPtr.NextEntryOffset + fileNameLen := int(dirInfoPtr.FileNameLength) / 2 + fileNameUTF16 := make([]uint16, fileNameLen) + targetPtr := unsafe.Pointer(&dirInfoPtr.FileName[0]) + for i := 0; i < fileNameLen; i++ { + fileNameUTF16[i] = *(*uint16)(targetPtr) + targetPtr = unsafe.Pointer(uintptr(targetPtr) + uintptr(2)) + } + dirInfoPtr = (*windowsext.FILE_FULL_DIR_INFO)(unsafe.Pointer(uintptr(unsafe.Pointer(dirInfoPtr)) + uintptr(offset))) + + fileName := windows.UTF16ToString(fileNameUTF16) + if fileName == "." || fileName == ".." { + continue + } + names = append(names, fileName) } - names = append(names, fileName) + continue } + return names, nil } From ef37b8a2c6ba89654caac30b10139e44bda1302f Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Wed, 7 Feb 2024 16:37:08 +0100 Subject: [PATCH 7/8] Windows: Close handles - to avoid hardlink exhaustion --- pkg/filesystem/local_directory_windows.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/filesystem/local_directory_windows.go b/pkg/filesystem/local_directory_windows.go index 85553eed..b08fd390 100644 --- a/pkg/filesystem/local_directory_windows.go +++ b/pkg/filesystem/local_directory_windows.go @@ -415,6 +415,7 @@ func (d *localDirectory) lstat(name path.Component) (FileType, error) { if err != nil { return FileTypeOther, err } + defer windows.CloseHandle(handle) var fileInfo windows.ByHandleFileInformation err = windows.GetFileInformationByHandle(handle, &fileInfo) if err != nil { @@ -532,6 +533,7 @@ func (d *localDirectory) Readlink(name path.Component) (string, error) { if err != nil { return "", err } + defer windows.CloseHandle(handle) outBufferSize := uint32(512) outBuffer := make([]byte, outBufferSize) var returned uint32 From 97ec1e5e1b119d4c1e04b4e5539691d605d1e337 Mon Sep 17 00:00:00 2001 From: Nils Wireklint Date: Wed, 7 Feb 2024 16:37:08 +0100 Subject: [PATCH 8/8] Windows: hardlinks do not need write permission This fixes execution errors where some programs fail to execute while a source file is hardlinked into another scheduled actions. Many compilers restrict the sharing mode for the files they open, 'cl.exe' for instance does not allow other processes to have open WRITE handles to its source files, and fails with SHARING_VIOLATION if such handles are open. The hardlinking file fetcher does not need this permission and we solve many crashes by removing it. --- pkg/filesystem/local_directory_windows.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/filesystem/local_directory_windows.go b/pkg/filesystem/local_directory_windows.go index b08fd390..913d62d0 100644 --- a/pkg/filesystem/local_directory_windows.go +++ b/pkg/filesystem/local_directory_windows.go @@ -864,7 +864,7 @@ func buildFileLinkInfo(root windows.Handle, name []uint16) ([]byte, uint32) { func createNTFSHardlink(oldHandle windows.Handle, oldName string, newHandle windows.Handle, newName string) error { var handle windows.Handle - err := ntCreateFile(&handle, windows.FILE_GENERIC_READ|windows.FILE_GENERIC_WRITE, oldHandle, oldName, windows.FILE_OPEN, 0) + err := ntCreateFile(&handle, windows.FILE_GENERIC_READ, oldHandle, oldName, windows.FILE_OPEN, 0) if err != nil { return err }