WIP: handle writing to dest within the foreign item handler #3779

oli-obk · 2024-07-31T15:13:05Z

alternative to #3777

RalfJung · 2024-08-08T11:40:55Z

src/shims/unix/fs.rs

            Some(metadata) => metadata,
-            None => return Ok(Scalar::from_i32(-1)), // `FileMetadata` has set errno
+            None => return Ok(EmulateItemResult::NeedsReturn),


Suggested change

None => return Ok(EmulateItemResult::NeedsReturn),

None => return Ok(EmulateItemResult::NeedsReturn), // `FileMetadata::from_path` has set the return value

But this is a bit odd, seems cleaner to instead have from_path return a Result and do the errno+return handling here?

A bit annoying as I have to return a Result<T, Scalar> to be able to handle both EBADF and io::Error, but generally nicer indeed

src/shims/unix/fs.rs

src/helpers.rs

src/shims/unix/fs.rs

bors · 2024-08-27T19:06:41Z

☔ The latest upstream changes (presumably #3804) made this pull request unmergeable. Please resolve the merge conflicts.

…nt directly

oli-obk · 2024-09-28T11:58:15Z

if this is to your liking, I'll port everything

@rustbot ready

bors · 2024-10-01T06:34:12Z

☔ The latest upstream changes (presumably #3923) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung

I generally like it, modulo the comments here. Note that this has more overlap with #3930 than I thought, so we should be careful to avoid conflicts.

RalfJung · 2024-10-02T20:21:29Z

src/helpers.rs

+    ) -> InterpResult<'tcx> {
+        self.set_last_error(err)?;
+        self.eval_context_mut().write_int(-1, dest)
+    }


Both of these new functions are superseded by the helper that @tiif added and that I extended. You should be able to use set_last_error_and_return now (after a rebase).

RalfJung · 2024-10-02T20:22:22Z

src/helpers.rs

+    /// Helper function that consumes an `std::io::Result<i32>` and writes the `Ok` integer
+    /// to the destination. In case the result is an error, this function writes
+    /// `-1` and sets the last OS error accordingly.
+    fn write_io_result(


Please move this to the new io_error.rs file, and put it right below try_unwrap_io_result (since it extends that helper).

RalfJung · 2024-10-02T20:24:40Z

src/shims/unix/fs.rs

-            Some(metadata) => metadata,
-            None => return Ok(Scalar::from_i32(-1)), // `FileMetadata` has set errno
+            Ok(metadata) => metadata,
+            Err(e) => return this.set_last_err_and_return_neg1(e, dest),
        };


I get the feeling we will have this pattern a lot... given an io::Result<T>, call return this.set_last_error_and_return on the error path and unwrap the T otherwise. Do you think that'd be worth a try!-like macro?

RalfJung · 2024-10-02T20:25:37Z

src/shims/unix/fs.rs

@@ -1050,7 +1051,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
            this.reject_in_isolation("`readdir`", reject_with)?;
            let eacc = this.eval_libc("EBADF");
            this.set_last_error(eacc)?;
-            return Ok(Scalar::null_ptr(this));
+            return this.write_scalar(eacc, dest);


This changes behavior, doesn't it? We should be returning a null pointer but you made it return eacc now.

I should fix this one in a separate PR with a test that shows the behaviour change. The previous behaviour is buggy

I don't think it was. readdir returns null on error.

readdir_r works different, but this is readdir.

RalfJung · 2024-10-02T20:28:03Z

src/shims/unix/fs.rs

@@ -1710,9 +1709,9 @@ impl FileMetadata {
    fn from_fd_num<'tcx>(
        ecx: &mut MiriInterpCx<'tcx>,
        fd_num: i32,
-    ) -> InterpResult<'tcx, Option<FileMetadata>> {
+    ) -> InterpResult<'tcx, Result<FileMetadata, Scalar>> {


Please add a doc comment explaining the meaning of this return type. (Same for the next function.)

Not necessary, I can use your new error enum instead

Ah, even better :)

RalfJung · 2024-10-02T20:33:21Z

src/shims/unix/fs.rs

@@ -1148,14 +1150,14 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
        // Reject if isolation is enabled.
        if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op {
            this.reject_in_isolation("`readdir_r`", reject_with)?;
-            // Set error code as "EBADF" (bad fd)
-            return Ok(Scalar::from_i32(this.fd_not_found()?));
+            // return positive error number on error


Suggested change

// return positive error number on error

// Return positive error number on error (*without* setting errno!)

RalfJung · 2024-10-02T20:35:11Z

src/shims/unix/mem.rs

@@ -27,7 +27,8 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
        flags: &OpTy<'tcx>,
        fd: &OpTy<'tcx>,
        offset: i128,
-    ) -> InterpResult<'tcx, Scalar> {
+        dest: &MPlaceTy<'tcx>,
+    ) -> InterpResult<'tcx, EmulateItemResult> {


So these here now return EmulateItemResult but the ones in fs.rs do not. As I said here I'd prefer to avoid having the EmulateItemResult for functions that don't need it. It is extremely rare to need this.

Probably just an oversight, I wanted to change all of them back.

oli-obk force-pushed the error_writer2 branch from 36114f3 to 2918e74 Compare August 1, 2024 14:31

RalfJung reviewed Aug 8, 2024

View reviewed changes

RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Aug 17, 2024

oli-obk and others added 6 commits September 28, 2024 11:47

Handle writing to dest within the foreign item handler

17f6d2e

Move linux_statx to writing to dest directly

7dd5651

Add helpers for setting errors and writing -1

7981e3f

Don't create a scalar from an int just to write it again, write the i…

ec4bef5

…nt directly

Move some macos functions to writing to dest directly

c4d5754

Write -1 together with setting the global error value

f162e51

oli-obk force-pushed the error_writer2 branch from 2918e74 to f162e51 Compare September 28, 2024 11:57

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Sep 28, 2024

RalfJung reviewed Oct 2, 2024

View reviewed changes

noahmbright mentioned this pull request Oct 4, 2024

Replace set_last_error with set_last_error_and_return #3941

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: handle writing to dest within the foreign item handler #3779

WIP: handle writing to dest within the foreign item handler #3779

oli-obk commented Jul 31, 2024

RalfJung Aug 8, 2024

oli-obk Sep 28, 2024

bors commented Aug 27, 2024

oli-obk commented Sep 28, 2024

bors commented Oct 1, 2024

RalfJung left a comment

RalfJung Oct 2, 2024

RalfJung Oct 2, 2024

RalfJung Oct 2, 2024

RalfJung Oct 2, 2024

oli-obk Oct 2, 2024

RalfJung Oct 2, 2024

RalfJung Oct 2, 2024

oli-obk Oct 2, 2024

RalfJung Oct 2, 2024

RalfJung Oct 2, 2024

RalfJung Oct 2, 2024

oli-obk Oct 2, 2024

	None => return Ok(EmulateItemResult::NeedsReturn),
	None => return Ok(EmulateItemResult::NeedsReturn), // `FileMetadata::from_path` has set the return value

	// return positive error number on error
	// Return positive error number on error (without setting errno!)

WIP: handle writing to dest within the foreign item handler #3779

Are you sure you want to change the base?

WIP: handle writing to dest within the foreign item handler #3779

Conversation

oli-obk commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Aug 27, 2024

oli-obk commented Sep 28, 2024

bors commented Oct 1, 2024

RalfJung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment