diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b806f1e..2fa419c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,37 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] ## +### Added ### +- New `EXTRA_RUSTC_FLAGS` and `EXTRA_CARGO_FLAGS` variables have been added to + our `Makefile`, making it easier for packaging tools to adjust builds while + still using `make release`. +- `install.sh` now accepts `--rust-target` and `--rust-buildmode` as parameters + to make cross-compilation workflows easier to write (in particular, this is + needed for runc's release scripts). + +### Fixed ### +- Previously, `staticlib` builds of libpathrs (i.e., `libpathrs.a`) + inadvertently included symbol versioned symbols (`@@LIBPATHRS_X.Y`), which + would cause linker errors when trying to compile programs statically against + libpathrs. + + This has been resolved, but downstream users who build runc without using + `make release` will need to take care to ensure they correctly set the + `LIBPATHRS_CAPI_BUILDMODE` environment variable when building and build + `libpathrs.a` and `libpathrs.so` in **separate** `cargo build` (or `cargo + rustc`) invocations. This is mostly necessary due to [the lack of support for + `#[cfg(crate_type)]`][rust-issue20267]. +- `go-pathrs` now correctly builds on 32-bit architectures. +- When doing `procfs` operations, previously libpathrs would internally keep a + handle to `ProcfsBase` open during the entire operation (due to `Drop` + semantics in Rust) rather than closing the file descriptor as quickly as + possible. The file descriptor would be closed soon afterwards (and thus was + not a leak) but tools that search for file descriptor leaks (such as runc's + test suite) could incorrectly classify this as a leak. We now close this + `ProcfsBase` handle far more aggressively. + +[rust-issue20267]: https://github.com/rust-lang/rust/issues/20267 + ## [0.2.3] - 2026-01-29 ## > この閃きを俺は信じる！ diff --git a/Cargo.toml b/Cargo.toml index 96b20262..46689f75 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -48,7 +48,6 @@ rust-version = "1.63" maintenance = { status = "experimental" } [lib] -# MSRV(1.64): Replace with --crate-type={cdy,static}lib in Makefile. crate-type = ["rlib"] [features] diff --git a/Makefile b/Makefile index da5c053c..cd0760cf 100644 --- a/Makefile +++ b/Makefile @@ -48,36 +48,50 @@ CARGO_CHECK := $(call cargo_hack,$(CARGO),check) CARGO_CLIPPY := $(call cargo_hack,$(CARGO),clippy) CARGO_LLVM_COV := $(call cargo_hack,$(CARGO_NIGHTLY),llvm-cov) -RUSTC_FLAGS := -C panic=abort -CARGO_FLAGS ?= --features=capi +EXTRA_RUSTC_FLAGS ?= +RUSTC_FLAGS := -C panic=abort $(EXTRA_RUSTC_FLAGS) +EXTRA_CARGO_FLAGS ?= +CARGO_FLAGS := --features=capi $(EXTRA_CARGO_FLAGS) SRC_FILES = $(wildcard Cargo.*) $(shell find . -name '*.rs') .DEFAULT: debug .PHONY: debug -debug: target/debug - -target/debug: $(SRC_FILES) - # MSRV(1.64): Use cargo rustc --crate-type={cdy,static}lib. - RUSTFLAGS="$(RUSTC_FLAGS)" ./hack/with-crate-type.sh $(CARGO) build $(CARGO_FLAGS) - # For some reason, --crate-type needs separate invocations. We can't use - # #![crate_type] unfortunately, as using it with #![cfg_attr] has been - # deprecated. - #$(CARGO) rustc $(CARGO_FLAGS) --crate-type=cdylib $(RUSTC_FLAGS) - #$(CARGO) rustc $(CARGO_FLAGS) --crate-type=staticlib $(RUSTC_FLAGS) +debug: target/debug/libpathrs.so target/debug/libpathrs.a + +target/debug/libpathrs.so: LIBPATHRS_CAPI_BUILDMODE := cdylib +target/debug/libpathrs.so: $(SRC_FILES) + LIBPATHRS_CAPI_BUILDMODE=$(LIBPATHRS_CAPI_BUILDMODE) \ + hack/with-crate-type.sh --crate-type=$(LIBPATHRS_CAPI_BUILDMODE) \ + env RUSTFLAGS="$(RUSTC_FLAGS)" $(CARGO) build $(CARGO_FLAGS) + # MSRV(1.64): Switch to cargo rustc --crate-type=... + #LIBPATHRS_CAPI_BUILDMODE=$(LIBPATHRS_CAPI_BUILDMODE) $(CARGO) rustc $(CARGO_FLAGS) --crate-type=$(LIBPATHRS_CAPI_BUILDMODE) -- $(RUSTC_FLAGS) +target/debug/libpathrs.a: LIBPATHRS_CAPI_BUILDMODE := staticlib +target/debug/libpathrs.a: $(SRC_FILES) + LIBPATHRS_CAPI_BUILDMODE=$(LIBPATHRS_CAPI_BUILDMODE) \ + hack/with-crate-type.sh --crate-type=$(LIBPATHRS_CAPI_BUILDMODE) \ + env RUSTFLAGS="$(RUSTC_FLAGS)" $(CARGO) build $(CARGO_FLAGS) + # MSRV(1.64): Switch to cargo rustc --crate-type=... + #LIBPATHRS_CAPI_BUILDMODE=$(LIBPATHRS_CAPI_BUILDMODE) $(CARGO) rustc $(CARGO_FLAGS) --crate-type=$(LIBPATHRS_CAPI_BUILDMODE) -- $(RUSTC_FLAGS) .PHONY: release -release: target/release - -target/release: CARGO_FLAGS += --release -target/release: $(SRC_FILES) - # MSRV(1.64): Use cargo rustc --crate-type={cdy,static}lib. - RUSTFLAGS="$(RUSTC_FLAGS)" ./hack/with-crate-type.sh $(CARGO) build $(CARGO_FLAGS) - # For some reason, --crate-type needs separate invocations. We can't use - # #![crate_type] unfortunately, as using it with #![cfg_attr] has been - # deprecated. - #$(CARGO) rustc $(CARGO_FLAGS) --crate-type=cdylib $(RUSTC_FLAGS) - #$(CARGO) rustc $(CARGO_FLAGS) --crate-type=staticlib $(RUSTC_FLAGS) +release: CARGO_FLAGS += --release +release: target/release/libpathrs.so target/release/libpathrs.a + +target/release/libpathrs.so: LIBPATHRS_CAPI_BUILDMODE := cdylib +target/release/libpathrs.so: $(SRC_FILES) + LIBPATHRS_CAPI_BUILDMODE=$(LIBPATHRS_CAPI_BUILDMODE) \ + hack/with-crate-type.sh --crate-type=$(LIBPATHRS_CAPI_BUILDMODE) \ + env RUSTFLAGS="$(RUSTC_FLAGS)" $(CARGO) build $(CARGO_FLAGS) + # MSRV(1.64): Switch to cargo rustc --crate-type=... + #LIBPATHRS_CAPI_BUILDMODE=$(LIBPATHRS_CAPI_BUILDMODE) $(CARGO) rustc $(CARGO_FLAGS) --crate-type=$(LIBPATHRS_CAPI_BUILDMODE) -- $(RUSTC_FLAGS) +target/release/libpathrs.a: LIBPATHRS_CAPI_BUILDMODE := staticlib +target/release/libpathrs.a: $(SRC_FILES) + LIBPATHRS_CAPI_BUILDMODE=$(LIBPATHRS_CAPI_BUILDMODE) \ + hack/with-crate-type.sh --crate-type=$(LIBPATHRS_CAPI_BUILDMODE) \ + env RUSTFLAGS="$(RUSTC_FLAGS)" $(CARGO) build $(CARGO_FLAGS) + # MSRV(1.64): Switch to cargo rustc --crate-type=... + #LIBPATHRS_CAPI_BUILDMODE=$(LIBPATHRS_CAPI_BUILDMODE) $(CARGO) rustc $(CARGO_FLAGS) --crate-type=$(LIBPATHRS_CAPI_BUILDMODE) -- $(RUSTC_FLAGS) .PHONY: smoke-test smoke-test: diff --git a/build.rs b/build.rs index 51c726da..2457ecaa 100644 --- a/build.rs +++ b/build.rs @@ -35,55 +35,76 @@ use std::{env, io::Write}; use tempfile::NamedTempFile; fn main() { - // Add DT_SONAME and other ELF metadata to our cdylibs. We can't check the - // crate-type here directly, but we can at least avoid needless warnings for - // "cargo build" by only emitting this when the capi feature is enabled. + println!("cargo:rerun-if-env-changed=CARGO_FEATURE_CAPI"); + println!("cargo:rerun-if-env-changed=LIBPATHRS_CAPI_BUILDMODE"); if cfg!(feature = "capi") { - let name = "pathrs"; - // TODO: Since we use symbol versioning, it seems quite unlikely that we - // would ever bump the major version in the SONAME, so we should - // probably hard-code this or define it elsewhere. - let major = env::var("CARGO_PKG_VERSION_MAJOR").unwrap(); - println!("cargo:rustc-cdylib-link-arg=-Wl,-soname,lib{name}.so.{major}"); + // Add DT_SONAME and other ELF metadata to our cdylibs. We can't check + // the crate-type here directly, so we have our Makefile define a + // special environment variable instead (for whatever reason, even + // --cfg=... doesn't seem to propagate to build.rs. This means that each + // crate-type needs to be built separately (with --cargo-type). + // + // The alternative would be to make cdylib/symvers a feature, which + // seems even more prone to breakage -- at least your build will + // explicitly fail if you don't specifying LIBPATHRS_CAPI_BUILDMODE. + let is_cdylib = match env::var("LIBPATHRS_CAPI_BUILDMODE") + .unwrap_or_else(|_| "".to_string()) + .as_str() + { + "cdylib" => { + println!("cargo:rustc-cfg=cdylib"); + true + } + "staticlib" | "" => false, + v => panic!("unknown LIBPATHRS_CAPI_BUILDMODE={v} value"), + }; + if is_cdylib { + let name = "pathrs"; + // TODO: Since we use symbol versioning, it seems quite unlikely that we + // would ever bump the major version in the SONAME, so we should + // probably hard-code this or define it elsewhere. + let major = env::var("CARGO_PKG_VERSION_MAJOR").unwrap(); + println!("cargo:rustc-cdylib-link-arg=-Wl,-soname,lib{name}.so.{major}"); - let (mut version_script_file, version_script_path) = - NamedTempFile::with_prefix("libpathrs-version-script.") - .expect("mktemp") - .keep() - .expect("persist mktemp"); - let version_script_path = version_script_path - .to_str() - .expect("mktemp should be utf-8 safe string"); - writeln!( - version_script_file, - // All of the symbol versions are done with in-line .symver entries. - // This version script is only needed to define the version nodes - // (and their dependencies). - // FIXME: "local" doesn't appear to actually hide symbols in the - // output .so. For more information about getting all of this to - // work nicely, see . - r#" + let (mut version_script_file, version_script_path) = + NamedTempFile::with_prefix("libpathrs-version-script.") + .expect("mktemp") + .keep() + .expect("persist mktemp"); + let version_script_path = version_script_path + .to_str() + .expect("mktemp should be utf-8 safe string"); + writeln!( + version_script_file, + // All of the symbol versions are done with in-line .symver entries. + // This version script is only needed to define the version nodes + // (and their dependencies). + // FIXME: "local" doesn't appear to actually hide symbols in the + // output .so. For more information about getting all of this to + // work nicely, see . + r#" LIBPATHRS_0.1 {{ }}; LIBPATHRS_0.2 {{ local: *; }} LIBPATHRS_0.1; "# - ) - .expect("write version script"); - println!("cargo:rustc-cdylib-link-arg=-Wl,--version-script={version_script_path}"); + ) + .expect("write version script"); + println!("cargo:rustc-cdylib-link-arg=-Wl,--version-script={version_script_path}"); - // The above version script (and our .symver setup) conflicts with the - // version script and options used by Rust when linking with GNU ld. - // Thankfully, lld Just Works(TM) out of the box so we can use it. - // - // Rust 1.90 switched to lld by default for x86, but for older versions - // and other architectures it is necessary to specify the linker as lld - // (there was also a rustflag for this but it was unstable until Rust - // 1.90). - // - // Unfortunately, while there are some clever tricks you could use for - // GNU ld (such as writing an ld wrapper and executing it with "cc -B"), - // doing so produces useless symbol versions so it's better to just - // require lld. Debian bullseye and later all have lld, so this is a - // non-issue for packagers. - println!("cargo:rustc-cdylib-link-arg=-fuse-ld=lld"); + // The above version script (and our .symver setup) conflicts with the + // version script and options used by Rust when linking with GNU ld. + // Thankfully, lld Just Works(TM) out of the box so we can use it. + // + // Rust 1.90 switched to lld by default for x86, but for older versions + // and other architectures it is necessary to specify the linker as lld + // (there was also a rustflag for this but it was unstable until Rust + // 1.90). + // + // Unfortunately, while there are some clever tricks you could use for + // GNU ld (such as writing an ld wrapper and executing it with "cc -B"), + // doing so produces useless symbol versions so it's better to just + // require lld. Debian bullseye and later all have lld, so this is a + // non-issue for packagers. + println!("cargo:rustc-cdylib-link-arg=-fuse-ld=lld"); + } } } diff --git a/go-pathrs/internal/libpathrs/libpathrs_linux.go b/go-pathrs/internal/libpathrs/libpathrs_linux.go index e8ca2a13..d54497a5 100644 --- a/go-pathrs/internal/libpathrs/libpathrs_linux.go +++ b/go-pathrs/internal/libpathrs/libpathrs_linux.go @@ -100,7 +100,7 @@ func InRootReadlink(rootFd uintptr, path string) (string, error) { size := 128 for { linkBuf := make([]byte, size) - n := C.pathrs_inroot_readlink(C.int(rootFd), cPath, C.cast_ptr(unsafe.Pointer(&linkBuf[0])), C.ulong(len(linkBuf))) + n := C.pathrs_inroot_readlink(C.int(rootFd), cPath, C.cast_ptr(unsafe.Pointer(&linkBuf[0])), C.size_t(len(linkBuf))) switch { case int(n) < C.__PATHRS_MAX_ERR_VALUE: return "", fetchError(n) @@ -301,7 +301,7 @@ func ProcReadlinkat(procRootFd int, base ProcBase, path string) (string, error) linkBuf := make([]byte, size) n := C.pathrs_proc_readlinkat( C.int(procRootFd), cBase, cPath, - C.cast_ptr(unsafe.Pointer(&linkBuf[0])), C.ulong(len(linkBuf))) + C.cast_ptr(unsafe.Pointer(&linkBuf[0])), C.size_t(len(linkBuf))) switch { case int(n) < C.__PATHRS_MAX_ERR_VALUE: return "", fetchError(n) diff --git a/go-pathrs/procfs/procfs_linux.go b/go-pathrs/procfs/procfs_linux.go index 14c861da..915e9ccd 100644 --- a/go-pathrs/procfs/procfs_linux.go +++ b/go-pathrs/procfs/procfs_linux.go @@ -56,10 +56,11 @@ var ( // *before* you call wait(2)or any equivalent method that could reap // zombies). func ProcPid(pid int) ProcBase { - if pid < 0 || pid >= 1<<31 { + if pid < 0 || uint64(pid) >= 1<<31 { panic("invalid ProcBasePid value") // TODO: should this be an error? } - return ProcBase{inner: libpathrs.ProcPid(uint32(pid))} + pid32 := uint32(pid) //nolint:gosec // G115 false positive + return ProcBase{inner: libpathrs.ProcPid(pid32)} } // ThreadCloser is a callback that needs to be called when you are done diff --git a/hack/with-crate-type.sh b/hack/with-crate-type.sh index 75c318e5..6a768e27 100755 --- a/hack/with-crate-type.sh +++ b/hack/with-crate-type.sh @@ -35,12 +35,32 @@ set -Eeuo pipefail +bail() { + echo "$*" >&2 + exit 1 +} + SRC_ROOT="$(readlink -f "$(dirname "${BASH_SOURCE[0]}")/..")" -backup="$(mktemp "$SRC_ROOT/Cargo.toml.XXXXXX")" + +# Only parse the first argument if it is --crate-type=foo. The rest of the +# arguments get passed as-is. +crate_types=("rlib" "staticlib" "cdylib") +case "${1:-}" in + --crate-type=*) + arg="${1#*=}" + IFS=', ' read -r -a crate_types <<<"$arg" + shift 1 + ;; +esac + +[ "$#" -gt 0 ] || bail "usage: with-crate-type.sh [--crate-type=...] [...]" set -x -sed -i".${backup##*.}" '/^crate-type/ s/=.*/= ["rlib", "cdylib", "staticlib"]/' "$SRC_ROOT/Cargo.toml" +backup="$(mktemp "$SRC_ROOT/Cargo.toml.XXXXXX")" +sed -i".${backup##*.}" \ + "/^crate-type/ s/=.*/= [$(printf '"%s",' "${crate_types[@]}")]/" \ + "$SRC_ROOT/Cargo.toml" # shellcheck disable=SC2064 # We want to expand the variables immediately. trap "mv '$backup' '$SRC_ROOT/Cargo.toml'" EXIT diff --git a/install.sh b/install.sh index 168aedc7..37f96034 100755 --- a/install.sh +++ b/install.sh @@ -54,6 +54,7 @@ SOVERSION="$(get_so_version)" SONAME="lib$(get_crate_info name).so.$FULLVERSION" +TARGET= # Try to emulate autoconf's basic flags. DEFAULT_PREFIX=/usr/local usage() { @@ -83,6 +84,13 @@ usage() { filesystem. This is usually used for distribution packaging. You can also pass environment variables as command-line arguments. + In addition, in order to support cross-compilation and custom build modes, + you can specify which rust target architecture and build mode to copy the + files from (rather than the top-level native target/release directory). + + --rust-target=[${TARGET:=}] + --rust-buildmode=[release] + Example: In an openSUSE rpm spec, this script could be used like this: @@ -103,7 +111,7 @@ usage() { [ "$#" -gt 0 ] && exit_code=1 exit "$exit_code" } -GETOPT="$(getopt -o h --long help,prefix:,exec-prefix:,includedir:,libdir:,pkgconfigdir: -- "$@")" +GETOPT="$(getopt -o h --long help,prefix:,exec-prefix:,includedir:,libdir:,pkgconfigdir:,rust-target:,rust-buildmode: -- "$@")" eval set -- "$GETOPT" DESTDIR="${DESTDIR:-}" @@ -112,19 +120,23 @@ exec_prefix= includedir= libdir= pkgconfigdir= +rust_target="$TARGET" +rust_buildmode=release while true; do case "$1" in - --prefix) prefix="$2"; shift 2 ;; - --exec-prefix) exec_prefix="$2"; shift 2 ;; - --includedir) includedir="$2"; shift 2 ;; - --libdir) libdir="$2"; shift 2 ;; - --pkgconfigdir) pkgconfigdir="$2"; shift 2 ;; + --prefix) prefix="$2"; shift 2 ;; + --exec-prefix) exec_prefix="$2"; shift 2 ;; + --includedir) includedir="$2"; shift 2 ;; + --libdir) libdir="$2"; shift 2 ;; + --pkgconfigdir) pkgconfigdir="$2"; shift 2 ;; + --rust-target) rust_target="$2"; shift 2 ;; + --rust-buildmode) rust_buildmode="$2"; shift 2 ;; --) shift; break ;; -h | --help) usage ;; *) usage "unknown argument $1" ;; esac done - +builddir="target/$rust_target/$rust_buildmode" for extra_arg in "$@"; do if [[ "$extra_arg" = *=* ]]; then @@ -210,8 +222,8 @@ set -x install -Dt "$DESTDIR/$pkgconfigdir/" -m 0644 pathrs.pc install -Dt "$DESTDIR/$includedir/" -m 0644 include/pathrs.h # Static library. -install -Dt "$DESTDIR/$libdir" -m 0644 target/release/libpathrs.a +install -Dt "$DESTDIR/$libdir" -m 0644 "$builddir/libpathrs.a" # Shared library. -install -DT -m 0755 target/release/libpathrs.so "$DESTDIR/$libdir/$SONAME" +install -DT -m 0755 "$builddir/libpathrs.so" "$DESTDIR/$libdir/$SONAME" ln -sf "$SONAME" "$DESTDIR/$libdir/libpathrs.so.$SOVERSION" ln -sf "$SONAME" "$DESTDIR/$libdir/libpathrs.so" diff --git a/src/capi/utils.rs b/src/capi/utils.rs index ab6b9a68..187c692a 100644 --- a/src/capi/utils.rs +++ b/src/capi/utils.rs @@ -73,6 +73,8 @@ use libc::{c_char, c_int, size_t}; macro_rules! symver { () => {}; (@with-meta $(#[$meta:meta])* $($block:tt)+) => { + // Only generate .symver entries for cdylib. + #[cfg(cdylib)] // Some architectures still have unstable ASM, which stops us from // injecting the ".symver" section. You can see the list in // LoweringContext::lower_inline_asm (compiler/rustc_asm_lowering). diff --git a/src/procfs.rs b/src/procfs.rs index 38e81c89..b646cd33 100644 --- a/src/procfs.rs +++ b/src/procfs.rs @@ -513,8 +513,7 @@ impl<'fd> ProcfsHandleRef<'fd> { // present because of subset=pid and retry (for magic-links we need to // operate on the target path more than once, which makes the retry // logic easier to do upfront here). - let basedir = self.open_base(base)?; - match self.openat_raw(basedir.as_fd(), subpath, oflags) { + match self.openat_raw(self.open_base(base)?.as_fd(), subpath, oflags) { Ok(file) => return Ok(file.into()), Err(err) => { if self.is_subset && err.kind() == ErrorKind::OsError(Some(libc::ENOENT)) { @@ -629,10 +628,9 @@ impl<'fd> ProcfsHandleRef<'fd> { oflags.insert(OpenFlags::O_NOFOLLOW); // Do a basic lookup. - let basedir = self.open_base(base)?; let subpath = subpath.as_ref(); let fd = self - .openat_raw(basedir.as_fd(), subpath, oflags) + .openat_raw(self.open_base(base)?.as_fd(), subpath, oflags) .or_else(|err| { if self.is_subset && err.kind() == ErrorKind::OsError(Some(libc::ENOENT)) { // If the lookup failed due to ENOENT, and the current