Skip to content

libc-misc test freebsd fixes attempt #3177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ case $HOST_TARGET in
MIRI_TEST_TARGET=aarch64-unknown-linux-gnu run_tests
MIRI_TEST_TARGET=aarch64-apple-darwin run_tests
MIRI_TEST_TARGET=i686-pc-windows-gnu run_tests
MIRI_TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal hello integer vec panic/panic concurrency/simple pthread-threadname libc-getentropy libc-getrandom libc-misc atomic env align
MIRI_TEST_TARGET=x86_64-unknown-freebsd run_tests_minimal hello integer vec panic/panic concurrency/simple pthread-threadname libc-getentropy libc-getrandom libc-misc libc-fs atomic env align
MIRI_TEST_TARGET=aarch64-linux-android run_tests_minimal hello integer vec panic/panic
MIRI_TEST_TARGET=wasm32-wasi run_tests_minimal no_std integer strings wasm
MIRI_TEST_TARGET=wasm32-unknown-unknown run_tests_minimal no_std integer strings wasm
Expand Down
4 changes: 2 additions & 2 deletions src/shims/time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
let mut relative_clocks;

match this.tcx.sess.target.os.as_ref() {
"linux" => {
// Linux has two main kinds of clocks. REALTIME clocks return the actual time since the
"linux" | "freebsd" => {
// Linux and FreeBSD have two main kinds of clocks. REALTIME clocks return the actual time since the
// Unix epoch, including effects which may cause time to move backwards such as NTP.
// Linux further distinguishes regular and "coarse" clocks, but the "coarse" version
// is just specified to be "faster and less precise", so we implement both the same way.
Expand Down
10 changes: 10 additions & 0 deletions src/shims/unix/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,16 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
"ftruncate64" => {
let [fd, length] =
this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let fd = this.read_scalar(fd)?.to_i32()?;
let length = this.read_scalar(length)?.to_i64()?;
let result = this.ftruncate64(fd, length)?;
this.write_scalar(result, dest)?;
}
"ftruncate" => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're adding this for all targets now we should probably have a test for it... libc-fs.rs sound like the right file.

We've historically been pretty bad with libc tests for our FS APIs though. And test_posix_mkstemp should probably be in libc-fs.rs as well. So if you prefer we can open an issue to track fixing this in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's still no test for this function, is there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not add purposely, can be done separately.

Copy link
Member

@RalfJung RalfJung Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I said it should either be done here or have an issue to track it. :)
I created an issue: #3179

let [fd, length] =
this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
let fd = this.read_scalar(fd)?.to_i32()?;
let length = this.read_target_isize(length)?;
let result = this.ftruncate64(fd, length)?;
this.write_scalar(result, dest)?;
}
Expand Down
9 changes: 1 addition & 8 deletions src/shims/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1504,16 +1504,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
}
}

fn ftruncate64(
&mut self,
fd_op: &OpTy<'tcx, Provenance>,
length_op: &OpTy<'tcx, Provenance>,
) -> InterpResult<'tcx, Scalar<Provenance>> {
fn ftruncate64(&mut self, fd: i32, length: i64) -> InterpResult<'tcx, Scalar<Provenance>> {
let this = self.eval_context_mut();

let fd = this.read_scalar(fd_op)?.to_i32()?;
let length = this.read_scalar(length_op)?.to_i64()?;

// Reject if isolation is enabled.
if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
this.reject_in_isolation("`ftruncate64`", reject_with)?;
Expand Down
7 changes: 0 additions & 7 deletions src/shims/unix/macos/foreign_items.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriInterpCxExt<'mir, 'tcx> {
let result = this.lseek64(fd, offset, whence)?;
this.write_scalar(result, dest)?;
}
"ftruncate" => {
let [fd, length] =
this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
// macOS is 64bit-only, so this is ftruncate64
let result = this.ftruncate64(fd, length)?;
this.write_scalar(result, dest)?;
}
"realpath$DARWIN_EXTSN" => {
let [path, resolved_path] =
this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?;
Expand Down
1 change: 1 addition & 0 deletions tests/pass-dep/shims/libc-fs-with-isolation.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
//@ignore-target-windows: no libc on Windows
//@ignore-target-freebsd: FIXME needs foreign function `stat@FBSD_1.0`
//@compile-flags: -Zmiri-isolation-error=warn-nobacktrace
//@normalize-stderr-test: "(stat(x)?)" -> "$$STAT"

Expand Down
43 changes: 43 additions & 0 deletions tests/pass-dep/shims/libc-fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ fn main() {
test_file_open_unix_extra_third_arg();
#[cfg(target_os = "linux")]
test_o_tmpfile_flag();
test_posix_mkstemp();
}

/// Prepare: compute filename and make sure the file does not exist.
Expand Down Expand Up @@ -151,3 +152,45 @@ fn test_o_tmpfile_flag() {
.raw_os_error(),
);
}

fn test_posix_mkstemp() {
use std::ffi::OsStr;
use std::os::unix::io::FromRawFd;
use std::path::Path;

let valid_template = "fooXXXXXX";
// C needs to own this as `mkstemp(3)` says:
// "Since it will be modified, `template` must not be a string constant, but
// should be declared as a character array."
// There seems to be no `as_mut_ptr` on `CString` so we need to use `into_raw`.
let ptr = CString::new(valid_template).unwrap().into_raw();
let fd = unsafe { libc::mkstemp(ptr) };
// Take ownership back in Rust to not leak memory.
let slice = unsafe { CString::from_raw(ptr) };
assert!(fd > 0);
let osstr = OsStr::from_bytes(slice.to_bytes());
let path: &Path = osstr.as_ref();
let name = path.file_name().unwrap().to_string_lossy();
assert!(name.ne("fooXXXXXX"));
assert!(name.starts_with("foo"));
assert_eq!(name.len(), 9);
assert_eq!(
name.chars().skip(3).filter(char::is_ascii_alphanumeric).collect::<Vec<char>>().len(),
6
);
let file = unsafe { File::from_raw_fd(fd) };
assert!(file.set_len(0).is_ok());

let invalid_templates = vec!["foo", "barXX", "XXXXXXbaz", "whatXXXXXXever", "X"];
for t in invalid_templates {
let ptr = CString::new(t).unwrap().into_raw();
let fd = unsafe { libc::mkstemp(ptr) };
let _ = unsafe { CString::from_raw(ptr) };
// "On error, -1 is returned, and errno is set to
// indicate the error"
assert_eq!(fd, -1);
let e = std::io::Error::last_os_error();
assert_eq!(e.raw_os_error(), Some(libc::EINVAL));
assert_eq!(e.kind(), std::io::ErrorKind::InvalidInput);
}
}
52 changes: 1 addition & 51 deletions tests/pass-dep/shims/libc-misc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,14 +172,13 @@ fn test_thread_local_errno() {
}

/// Tests whether clock support exists at all
#[cfg(not(target_os = "freebsd"))]
fn test_clocks() {
let mut tp = std::mem::MaybeUninit::<libc::timespec>::uninit();
let is_error = unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, tp.as_mut_ptr()) };
assert_eq!(is_error, 0);
let is_error = unsafe { libc::clock_gettime(libc::CLOCK_MONOTONIC, tp.as_mut_ptr()) };
assert_eq!(is_error, 0);
#[cfg(target_os = "linux")]
#[cfg(any(target_os = "linux", target_os = "freebsd"))]
{
let is_error = unsafe { libc::clock_gettime(libc::CLOCK_REALTIME_COARSE, tp.as_mut_ptr()) };
assert_eq!(is_error, 0);
Expand Down Expand Up @@ -238,51 +237,6 @@ fn test_isatty() {
}
}

#[cfg(not(target_os = "freebsd"))]
fn test_posix_mkstemp() {
use std::ffi::CString;
use std::ffi::OsStr;
use std::os::unix::ffi::OsStrExt;
use std::os::unix::io::FromRawFd;
use std::path::Path;

let valid_template = "fooXXXXXX";
// C needs to own this as `mkstemp(3)` says:
// "Since it will be modified, `template` must not be a string constant, but
// should be declared as a character array."
// There seems to be no `as_mut_ptr` on `CString` so we need to use `into_raw`.
let ptr = CString::new(valid_template).unwrap().into_raw();
let fd = unsafe { libc::mkstemp(ptr) };
// Take ownership back in Rust to not leak memory.
let slice = unsafe { CString::from_raw(ptr) };
assert!(fd > 0);
let osstr = OsStr::from_bytes(slice.to_bytes());
let path: &Path = osstr.as_ref();
let name = path.file_name().unwrap().to_string_lossy();
assert!(name.ne("fooXXXXXX"));
assert!(name.starts_with("foo"));
assert_eq!(name.len(), 9);
assert_eq!(
name.chars().skip(3).filter(char::is_ascii_alphanumeric).collect::<Vec<char>>().len(),
6
);
let file = unsafe { File::from_raw_fd(fd) };
assert!(file.set_len(0).is_ok());

let invalid_templates = vec!["foo", "barXX", "XXXXXXbaz", "whatXXXXXXever", "X"];
for t in invalid_templates {
let ptr = CString::new(t).unwrap().into_raw();
let fd = unsafe { libc::mkstemp(ptr) };
let _ = unsafe { CString::from_raw(ptr) };
// "On error, -1 is returned, and errno is set to
// indicate the error"
assert_eq!(fd, -1);
let e = std::io::Error::last_os_error();
assert_eq!(e.raw_os_error(), Some(libc::EINVAL));
assert_eq!(e.kind(), std::io::ErrorKind::InvalidInput);
}
}

fn test_memcpy() {
unsafe {
let src = [1i8, 2, 3];
Expand Down Expand Up @@ -406,9 +360,6 @@ fn test_reallocarray() {
fn main() {
test_posix_gettimeofday();

#[cfg(not(target_os = "freebsd"))] // FIXME we should support this on FreeBSD as well
test_posix_mkstemp();

test_posix_realpath_alloc();
test_posix_realpath_noalloc();
test_posix_realpath_errors();
Expand All @@ -417,7 +368,6 @@ fn main() {

test_isatty();

#[cfg(not(target_os = "freebsd"))] // FIXME we should support this on FreeBSD as well
test_clocks();

test_dlsym();
Expand Down