Skip to content

Conversation

CraftSpider
Copy link
Contributor

@CraftSpider CraftSpider commented Aug 29, 2025

Fixes #4483

@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2025

Thank you for contributing to Miri!
Please remember to not force-push to the PR branch except when you need to rebase due to a conflict or when the reviewer asks you for it.

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label Aug 29, 2025
@rustbot
Copy link
Collaborator

rustbot commented Aug 29, 2025

☔ The latest upstream changes made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Can you also add a File::create on a file that already exists somewhere suitable in this file so we ensure we cover that codepath?

@@ -209,7 +208,10 @@ fn test_file_set_len() {

// Can't use set_len on a file not opened for writing
let file = OpenOptions::new().read(true).open(&path).unwrap();
assert_eq!(ErrorKind::InvalidInput, file.set_len(14).unwrap_err().kind());
assert_eq!(
if cfg!(windows) { ErrorKind::PermissionDenied } else { ErrorKind::InvalidInput },
Copy link
Member

Choose a reason for hiding this comment

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

Since we pass through the host OS error, I doubt this will work properly. Just check that it's one of the two (with a comment explaining why).

Comment on lines +414 to +417
let ptr = this.deref_pointer_as(
file_information,
this.windows_ty_layout("FILE_END_OF_FILE_INFO"),
)?;
Copy link
Member

Choose a reason for hiding this comment

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

This re-reads the pointer. Since you already read the pointer, you can just call ptr_to_mplace directly.


let class = this.read_scalar(class)?.to_u32()?;
let buffer_size = this.read_scalar(buffer_size)?.to_u32()?;
let file_info = this.read_pointer(file_information)?;
Copy link
Member

Choose a reason for hiding this comment

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

Please pick a consistent name (file_information vs file_info).

CheckInAllocMsg::MemoryAccess,
)?;

let file = this.read_handle(file, "GetFileInformationByHandle")?;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let file = this.read_handle(file, "GetFileInformationByHandle")?;
let file = this.read_handle(file, "SetFileInformationByHandle")?;

Happens a few more times below.

@RalfJung
Copy link
Member

RalfJung commented Sep 3, 2025

@rustbot author

There's also a conflict that prevents CI from running, so please rebase to resolve that.

@rustbot 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 Sep 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Sep 3, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows: File::create fails when file already exists (cannot call SetFileInformationByHandle)
3 participants