Fix libc::read shim: make it write to a buffer correct amount of bytes. Add tests for new behavior #3720

safinaskar · 2024-06-29T01:20:00Z

libc::read shim had a bug: if underlying real call libc::read(fd, buf, N) returns M, then
libc::read shim writes N bytes to buf instead of M. Remaining N - M bytes are filled with zeros.
This commit fixes this bug and adds tests for new behavior

RalfJung · 2024-06-30T11:19:32Z

Thanks!
Could you please add a PR description explaining the goal of the PR? We already have tests of the file system APIs and if read did not initialize the buffer they would obviously fail.

It would be nice to have some tests that check the libc APIs (open/read/write/close) directly, basically a libc version of src/shims/unix/fs.rs -- see #3179. If that is the goal of this PR then the filename and test content should be adjusted accordingly. Specifically, this should be added in tests/pass-dep/shims/libc-fs.rs (after checking that none of the existing tests there already covers this).

safinaskar · 2024-06-30T19:17:21Z

@RalfJung , okay. I will remove draft status, when I'm done

RalfJung · 2024-06-30T19:29:22Z

I am not sure if that generates a notification... please write @rustbot ready when it is ready.

@rustbot author

safinaskar · 2024-06-30T19:36:29Z

@RalfJung , okay

safinaskar · 2024-07-05T21:54:58Z

@RalfJung
@rustbot ready
See updated title and description

RalfJung

Ah, that's what this is about. :) Good catch, and thanks for the PR!

RalfJung · 2024-07-06T09:30:20Z

src/shims/unix/fd.rs

@@ -394,7 +394,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
        match result {
            Ok(read_bytes) => {
                // If reading to `bytes` did not fail, we write those bytes to the buffer.
-                this.write_bytes_ptr(buf, bytes)?;
+                this.write_bytes_ptr(buf, bytes[..usize::try_from(read_bytes).unwrap()].to_vec())?;


Suggested change

this.write_bytes_ptr(buf, bytes[..usize::try_from(read_bytes).unwrap()].to_vec())?;

// Crucially, if fewer than `bytes.len()` bytes were read, only write

// that much into the output buffer!

this.write_bytes_ptr(buf, bytes[..usize::try_from(read_bytes).unwrap()].to_vec())?;

Also, to_vec should not be needed. .iter().copied() should work.

RalfJung · 2024-07-06T09:30:41Z

tests/fail-dep/libc/libc-read-and-uninit-premature-eof.rs

@@ -0,0 +1,25 @@
+// We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be initialized
+//@ignore-target-windows: we have no file deletion on Windows


Suggested change

//@ignore-target-windows: we have no file deletion on Windows

//@ignore-target-windows: no file system support on Windows

tests/fail-dep/libc/libc-read-and-uninit-premature-eof.rs

RalfJung · 2024-07-06T09:35:25Z

tests/fail-dep/libc/libc-read-and-uninit.rs

What is the point of this test? I don't think this is a likely enough error to justify spending CI time on.

RalfJung · 2024-07-06T09:36:32Z

tests/pass-dep/libc/libc-read-and-uninit.rs

Please move these into libc-fs.rs

RalfJung · 2024-07-06T20:11:50Z

@rustbot author

safinaskar · 2024-07-09T02:04:04Z

@RalfJung
@rustbot ready
I made all requested changes

RalfJung · 2024-07-09T05:49:50Z

tests/pass-dep/libc/libc-fs.rs

+        remove_file(&path).unwrap();
+    }
+    {
+        // We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be overwritten, and remaining byte will be left as is


Suggested change

// We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be overwritten, and remaining byte will be left as is

// We test that if we requested to read 4 bytes, but actually read 3 bytes, then

// 3 bytes (not 4) will be overwritten, and the remaining byte will be left as-is.

RalfJung · 2024-07-09T05:50:03Z

tests/pass-dep/libc/libc-fs.rs

+fn test_read_and_uninit() {
+    use std::mem::MaybeUninit;
+    {
+        // We test that libc::read initializes its buffer


Suggested change

// We test that libc::read initializes its buffer

// We test that libc::read initializes its buffer.

RalfJung · 2024-07-09T05:50:34Z

tests/fail-dep/libc/libc-read-and-uninit-premature-eof.rs

@@ -0,0 +1,26 @@
+// We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be initialized


Suggested change

// We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be initialized

//! We test that if we requested to read 4 bytes, but actually read 3 bytes,

//! then 3 bytes (not 4) will be initialized.

RalfJung · 2024-07-09T05:52:50Z

Just some final nits, please make these comments proper sentences and line-wrap them.

Usually I'd just apply these patches myself, but you seem to have disabled the option in the PR to let maintainers push to your branch. I can only recommend to enable that option, it saves you some work that maintainers can do for you. :)

@rustbot author

…s. Add tests for the new behavior. libc::read shim had a bug: if underlying real call libc::read(fd, buf, N) returns M, then libc::read shim writes N bytes to buf instead of M. Remaining N - M bytes are filled with zeros. This commit fixes this bug and adds tests for new behavior

safinaskar · 2024-07-09T20:50:07Z

@RalfJung
@rustbot ready
I made all requested changes

RalfJung · 2024-07-10T10:54:16Z

Thanks, looks good!
@bors r+

bors · 2024-07-10T10:54:19Z

📌 Commit e5c380b has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-07-10T10:55:26Z

⌛ Testing commit e5c380b with merge 7184789...

bors · 2024-07-10T11:19:10Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 7184789 to master...

safinaskar marked this pull request as draft June 29, 2024 01:20

rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jun 30, 2024

safinaskar changed the title ~~add tests for libc::read~~ Fix libc::read shim: make it write to a buffer correct amount of bytes. Add tests for new behavior Jul 5, 2024

safinaskar marked this pull request as ready for review July 5, 2024 21:54

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 Jul 5, 2024

RalfJung reviewed Jul 6, 2024

View reviewed changes

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

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 Jul 9, 2024

RalfJung reviewed Jul 9, 2024

View reviewed changes

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

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 Jul 9, 2024

bors merged commit 7184789 into rust-lang:master Jul 10, 2024
8 checks passed

safinaskar deleted the read branch July 10, 2024 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix libc::read shim: make it write to a buffer correct amount of bytes. Add tests for new behavior #3720

Fix libc::read shim: make it write to a buffer correct amount of bytes. Add tests for new behavior #3720

safinaskar commented Jun 29, 2024 •

edited

Loading

RalfJung commented Jun 30, 2024 •

edited

Loading

safinaskar commented Jun 30, 2024

RalfJung commented Jun 30, 2024

safinaskar commented Jun 30, 2024

safinaskar commented Jul 5, 2024

RalfJung left a comment

RalfJung Jul 6, 2024

RalfJung Jul 6, 2024

RalfJung Jul 6, 2024

RalfJung Jul 6, 2024

RalfJung commented Jul 6, 2024

safinaskar commented Jul 9, 2024

RalfJung Jul 9, 2024

RalfJung Jul 9, 2024

RalfJung Jul 9, 2024

RalfJung commented Jul 9, 2024

safinaskar commented Jul 9, 2024

RalfJung commented Jul 10, 2024

bors commented Jul 10, 2024

bors commented Jul 10, 2024

bors commented Jul 10, 2024

		@@ -0,0 +1,25 @@
		// We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be initialized
		//@ignore-target-windows: we have no file deletion on Windows

	//@ignore-target-windows: we have no file deletion on Windows
	//@ignore-target-windows: no file system support on Windows

	// We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be overwritten, and remaining byte will be left as is
	// We test that if we requested to read 4 bytes, but actually read 3 bytes, then
	// 3 bytes (not 4) will be overwritten, and the remaining byte will be left as-is.

	// We test that libc::read initializes its buffer
	// We test that libc::read initializes its buffer.

		@@ -0,0 +1,26 @@
		// We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be initialized

	// We test that if we requested to read 4 bytes, but actually read 3 bytes, then 3 bytes (not 4) will be initialized
	//! We test that if we requested to read 4 bytes, but actually read 3 bytes,
	//! then 3 bytes (not 4) will be initialized.

Fix libc::read shim: make it write to a buffer correct amount of bytes. Add tests for new behavior #3720

Fix libc::read shim: make it write to a buffer correct amount of bytes. Add tests for new behavior #3720

Conversation

safinaskar commented Jun 29, 2024 • edited Loading

RalfJung commented Jun 30, 2024 • edited Loading

safinaskar commented Jun 30, 2024

RalfJung commented Jun 30, 2024

safinaskar commented Jun 30, 2024

safinaskar commented Jul 5, 2024

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung Jul 6, 2024

Choose a reason for hiding this comment

RalfJung Jul 6, 2024

Choose a reason for hiding this comment

RalfJung Jul 6, 2024

Choose a reason for hiding this comment

RalfJung Jul 6, 2024

Choose a reason for hiding this comment

RalfJung commented Jul 6, 2024

safinaskar commented Jul 9, 2024

RalfJung Jul 9, 2024

Choose a reason for hiding this comment

RalfJung Jul 9, 2024

Choose a reason for hiding this comment

RalfJung Jul 9, 2024

Choose a reason for hiding this comment

RalfJung commented Jul 9, 2024

safinaskar commented Jul 9, 2024

RalfJung commented Jul 10, 2024

bors commented Jul 10, 2024

bors commented Jul 10, 2024

bors commented Jul 10, 2024

safinaskar commented Jun 29, 2024 •

edited

Loading

RalfJung commented Jun 30, 2024 •

edited

Loading