Skip to content

Error when there is an unsupported flag for opening a file #987

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 4 commits into from
Oct 16, 2019

Conversation

pvdrz
Copy link
Contributor

@pvdrz pvdrz commented Oct 11, 2019

@RalfJung this is my attempt to check for undesired flags. I also changed fcntl to error when doing any other action besides getting the flags for a fd

@pvdrz pvdrz mentioned this pull request Oct 11, 2019
@bors
Copy link
Contributor

bors commented Oct 11, 2019

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

@pvdrz pvdrz force-pushed the check-unsupported-fs-flags branch from a5dbdee to a1c6797 Compare October 11, 2019 16:43
@pvdrz pvdrz force-pushed the check-unsupported-fs-flags branch from 400ca87 to d73fae1 Compare October 11, 2019 17:20
@RalfJung
Copy link
Member

Hm... now the odd part is that CI works here, isn't it? When you do File::open(path), shouldn't this return something from F_GETFD that does not have FD_CLOEXEC set, and thus lead to an F_SETFD call and hence an error?

@RalfJung
Copy link
Member

(Also #973 still has some unresolved comments, not sure if you wanted to do separate PRs or add them here.)

@pvdrz
Copy link
Contributor Author

pvdrz commented Oct 13, 2019

Hm... now the odd part is that CI works here, isn't it? When you do File::open(path), shouldn't this return something from F_GETFD that does not have FD_CLOEXEC set, and thus lead to an F_SETFD call and hence an error?

Hmm yeah, that's odd. Maybe it happens because some of the O_* flags overlap with the FD_* flags and the bits for FD_CLOEXEC are set by pure luck. In that case I think I'll remove the flag field from the FIleHandle and return just FD_CLOEXEC given that this is always set by std.

Quote from man:

The following commands manipulate the flags associated with a file
descriptor. Currently, only one such flag is defined: FD_CLOEXEC,
the close-on-exec flag.

So it is ok to just return FD_CLOEXEC but I'll add some comments for this.

EDIT: Should I just remove the FileHandle type completely?

@pvdrz pvdrz force-pushed the check-unsupported-fs-flags branch from 49fc9c8 to f76f8ce Compare October 13, 2019 01:42
@pvdrz
Copy link
Contributor Author

pvdrz commented Oct 13, 2019

(Also #973 still has some unresolved comments, not sure if you wanted to do separate PRs or add them here.)

I think I won't be adding more changes to this PR. I'll take care of those comments after the changes to make public the memory field are merged.

@pvdrz pvdrz force-pushed the check-unsupported-fs-flags branch from 8b032f5 to 2487223 Compare October 15, 2019 13:01
@RalfJung
Copy link
Member

Thanks a lot! @bors r+

As a follow-up, one thing that would be nice to do is add some tests that trigger BADFD to catch errors like this one.

@bors
Copy link
Contributor

bors commented Oct 16, 2019

📌 Commit 2487223 has been approved by RalfJung

bors added a commit that referenced this pull request Oct 16, 2019
…lfJung

Error when there is an unsupported flag for opening a file

@RalfJung this is my attempt to check for undesired flags. I also changed fcntl to error when doing any other action besides getting the flags for a fd
@bors
Copy link
Contributor

bors commented Oct 16, 2019

⌛ Testing commit 2487223 with merge 49cab51...

@bors
Copy link
Contributor

bors commented Oct 16, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 49cab51 to master...

@bors bors merged commit 2487223 into rust-lang:master Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants