Safer poll timeout by JonathanWoollett-Light · Pull Request #1876 · nix-rust/nix

JonathanWoollett-Light · 2022-11-21T00:38:21Z

Implements a wrapper around i32 for the timeout argument in poll::poll.

This prevents the error case:

EINVAL (ppoll()) The timeout value expressed in *ip is invalid
(negative).

Most of the code additions are simple conversions to/from integer primitives.

An alternative approach to implement the same safety with less code would require something like https://internals.rust-lang.org/t/non-negative-integer-types/17796 and would offer less usability like converting to/from std::time::Durations.

asomers · 2022-11-23T20:37:15Z

CI is failing because you're trying to use rust 1.58.0, but the 1.58 container installs 1.58.1. Change the VERSION to 1.58.1

JonathanWoollett-Light · 2022-11-23T20:44:00Z

CI is failing because you're trying to use rust 1.58.0, but the 1.58 container installs 1.58.1. Change the VERSION to 1.58.1

Thank you for letting me know.

Why is this the case?

asomers · 2022-11-23T20:49:22Z

CI is failing because you're trying to use rust 1.58.0, but the 1.58 container installs 1.58.1. Change the VERSION to 1.58.1

Thank you for letting me know.

Why is this the case?

That's just the convention for the rust-lang containers. They always include the newest patch release.

asomers · 2022-12-04T22:10:33Z

src/poll.rs

+// specialization.
+impl PollTimeout {
+    /// Blocks indefinitely.
+    pub const NONE: Self = Self(1 << 31);


Is this supposed to be C's INFTIM, and if so shouldn't it be -1?

I think I was originally using a unsigned integer type here for some reason e.g. 1 << 31 == -1. But it makes more sense to use -1 now, will change this.

asomers · 2022-12-04T22:14:54Z

src/poll.rs

@@ -132,12 +384,12 @@ libc_bitflags! {
 /// in timeout means an infinite timeout.  Specifying a timeout of zero


You should update the docs for the timeout variable.

Updated docs

asomers · 2022-12-04T22:15:31Z

CHANGELOG.md

- The MSRV is now 1.56.1
-  ([#1792](https://github.com/nix-rust/nix/pull/1792))
+- The MSRV is now 1.58.1
+  ([#1876](https://github.com/nix-rust/nix/pull/1876))


You can delete this part, and all of the other MSRV stuff, since we'll merge #1862 first.

Dropped this commit

asomers · 2022-12-10T16:54:45Z

src/poll.rs

+
+/// Error type for [`PollTimeout::try_from::<i68>()`].
+#[derive(Debug, Clone, Copy)]
+pub enum TryFromI64Error {


I think TryFromI128Error and TryFromI64Error are exactly the same. Could you use a single error type?

Made some changes to the conversions to remove this error type.

asomers · 2022-12-10T17:00:07Z

src/poll.rs

+    type Error = ();
+    fn try_from(x: PollTimeout) -> std::result::Result<Self, ()> {
+        match x.timeout() {
+            // SAFETY: `x.0` is always positive.


Suggested change

// SAFETY: `x.0` is always positive.

// SAFETY: `x.0` is always nonnegative.

asomers · 2022-12-10T17:01:07Z

src/poll.rs

+    pub const MAX: Self = Self(i32::MAX);
+    /// Returns if `self` equals [`PollTimeout::NONE`].
+    pub fn is_none(&self) -> bool {
+        *self == Self::NONE


Just to program defensively, I recommend:

Suggested change

*self == Self::NONE

*self <= Self::NONE

You've right, fixed it.

asomers · 2022-12-10T17:03:03Z

src/poll.rs

+/// in timeout means an infinite timeout.  Specifying a timeout of
+/// [`PollTimeout::ZERO`] causes `poll()` to return immediately, even if no file
+/// descriptors are ready.
+pub fn poll(fds: &mut [PollFd], timeout: PollTimeout) -> Result<libc::c_int> {


I think it would be more usable, and more backwards compatible, to write the signature like this:

Suggested change

pub fn poll(fds: &mut [PollFd], timeout: PollTimeout) -> Result<libc::c_int> {

pub fn poll<T: Into<PollTimeout>>(fds: &mut [PollFd], timeout: T) -> Result<libc::c_int> {

Makes sense, made the change.

JonathanWoollett-Light · 2023-10-04T15:49:57Z

BTW, CI is failing due the an unused import:

// test_poll.rs

error: unused import: `close`
 --> test/test_poll.rs:4:14
  |
4 |     unistd::{close, pipe, write},
  |              ^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`

error: could not compile `nix` due to previous error

Fixed

asomers · 2023-11-04T16:18:52Z

src/poll.rs

+        !self.is_none()
+    }
+    /// Returns the timeout in milliseconds if there is some, otherwise returns `None`.
+    pub fn timeout(&self) -> Option<i32> {


I think naming this method as_millis would be more idiomatic. For example, it would mirror Duration::as_millis. And since it will never return a negative value, should it return u32 instead of i32?

I've split this into 2 functions:

/// Returns the timeout in milliseconds if there is some, otherwise returns `None`. pub fn as_millis(&self) -> Option<i32> { self.is_some().then_some(self.0) } /// Returns the timeout as a `Duration` if there is some, otherwise returns `None`. pub fn timeout(&self) -> Option<Duration> { self.as_millis().map(|x|Duration::from_millis(u64::try_from(x).unwrap())) }

I think using i32 or u32 each have their own drawbacks here, ideally we could use u31 with something like https://crates.io/crates/ux but this crate is not actively maintained. I tried my hand at making a better version as an experiment with https://crates.io/crates/ux2 but I am busy so am not able to maintain this alone.

I agree that u31 suits best here, but before switching to the ux or ux2 crate, u32 is the better choice compared to i32

What about this one?

I am okay to use u32.

Updated to u32.

asomers · 2023-11-04T16:23:23Z

src/poll.rs

+impl TryFrom<i128> for PollTimeout {
+    type Error = <i32 as TryFrom<i128>>::Error;
+    fn try_from(x: i128) -> std::result::Result<Self, Self::Error> {
+        match x {
+            // > Specifying a negative value in timeout means an infinite timeout.
+            i128::MIN..=-1 => Ok(Self::NONE),
+            millis @ 0.. => Ok(Self(i32::try_from(millis)?)),
+        }
+    }
+}
+impl TryFrom<i64> for PollTimeout {
+    type Error = <i32 as TryFrom<i64>>::Error;
+    fn try_from(x: i64) -> std::result::Result<Self, Self::Error> {
+        match x {
+            i64::MIN..=-1 => Ok(Self::NONE),
+            millis @ 0.. => Ok(Self(i32::try_from(millis)?)),
+        }
+    }
+}
+impl From<i32> for PollTimeout {
+    fn from(x: i32) -> Self {
+        Self(x)
+    }
+}
+impl From<i16> for PollTimeout {
+    fn from(x: i16) -> Self {
+        Self(i32::from(x))
+    }
+}
+impl From<i8> for PollTimeout {
+    fn from(x: i8) -> Self {
+        Self(i32::from(x))
+    }
+}


Actually, accepting negative values for timeout seems to be a Linuxism. POSIX only requires that -1 be interpreted as "forever", and FreeBSD will return EINVAL for other negative values. Please make it impossible to contruct a PollTimeout with a negative value other than -1.

SteveLauC · 2023-11-07T03:57:37Z

BTW, we have changed our changelog mode, please see CONTRIBUTING.md on how to add changelog entries

src/poll.rs

SteveLauC · 2023-11-20T11:19:43Z

Left some comments, I think this PR is ready for merge after addressing these comments

SteveLauC · 2023-11-22T01:48:34Z

The CI failure is not related to this PR

asomers · 2023-11-23T00:05:32Z

I can't reproduce the failure. Could github somehow be using a cached Cargo.lock file?

SteveLauC · 2023-11-23T00:09:37Z

I can't reproduce the failure. Could github somehow be using a cached Cargo.lock file?

Try this? I can reproduce it even with an empty hello-world crate

asomers · 2023-11-23T00:15:03Z

Ahh, so it's a toolchain bug. I thought it was a libc bug. I'll open a bug upstream. In the meantime, we'll either have to disable this target, or pin our rustc version.

SteveLauC · 2023-11-23T00:16:19Z

Ahh, so it's a toolchain bug. I thought it was a libc bug. I'll open a bug upstream. In the meantime, we'll either have to disable this target, or pin our rustc version.

I will pin the nightly toolchain to the version just before that

JonathanWoollett-Light force-pushed the poll-timeout branch 8 times, most recently from 74f8396 to ed82f41 Compare November 23, 2022 20:31

JonathanWoollett-Light force-pushed the poll-timeout branch 4 times, most recently from ed39c68 to 297c74c Compare November 23, 2022 20:43

asomers added this to the 0.27.0 milestone Nov 29, 2022

asomers requested changes Dec 4, 2022

View reviewed changes

JonathanWoollett-Light force-pushed the poll-timeout branch 4 times, most recently from a921a5d to 749d693 Compare December 10, 2022 13:01

asomers reviewed Dec 10, 2022

View reviewed changes

JonathanWoollett-Light force-pushed the poll-timeout branch 2 times, most recently from 7c63be2 to f11b599 Compare December 11, 2022 08:22

JonathanWoollett-Light force-pushed the poll-timeout branch 2 times, most recently from 3786bff to 81794fe Compare December 18, 2022 13:17

JonathanWoollett-Light force-pushed the poll-timeout branch 2 times, most recently from 297be36 to 0d3316d Compare December 31, 2022 22:34

JonathanWoollett-Light force-pushed the poll-timeout branch 2 times, most recently from 899f471 to 8852303 Compare January 13, 2023 21:54

asomers requested changes Nov 4, 2023

View reviewed changes

JonathanWoollett-Light force-pushed the poll-timeout branch from 5ab2dcd to e6b1a07 Compare November 7, 2023 03:34

JonathanWoollett-Light force-pushed the poll-timeout branch 7 times, most recently from 058899a to 776aaf1 Compare November 7, 2023 07:58

JonathanWoollett-Light force-pushed the poll-timeout branch 2 times, most recently from 422f03c to a8bcce3 Compare November 16, 2023 18:08

SteveLauC requested changes Nov 20, 2023

View reviewed changes

src/poll.rs Outdated Show resolved Hide resolved

src/poll.rs Show resolved Hide resolved

JonathanWoollett-Light force-pushed the poll-timeout branch 2 times, most recently from 42bdd0e to 167d4be Compare November 21, 2023 20:37

feat: Safer poll timeout

80c2d82

JonathanWoollett-Light force-pushed the poll-timeout branch from 167d4be to 80c2d82 Compare November 21, 2023 20:54

SteveLauC approved these changes Nov 22, 2023

View reviewed changes

JonathanWoollett-Light mentioned this pull request Nov 23, 2023

Epoll timeout #2202

Merged

3 tasks

SteveLauC added this pull request to the merge queue Nov 23, 2023

Merged via the queue into nix-rust:master with commit 7e16e32 Nov 23, 2023

SteveLauC mentioned this pull request Dec 15, 2023

listen does not allow specifying a negative backlog #2264

Closed

		@@ -132,12 +384,12 @@ libc_bitflags! {
		/// in timeout means an infinite timeout. Specifying a timeout of zero

	// SAFETY: `x.0` is always positive.
	// SAFETY: `x.0` is always nonnegative.

	pub fn poll(fds: &mut [PollFd], timeout: PollTimeout) -> Result<libc::c_int> {
	pub fn poll<T: Into<PollTimeout>>(fds: &mut [PollFd], timeout: T) -> Result<libc::c_int> {

Conversation

JonathanWoollett-Light commented Nov 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asomers commented Nov 23, 2022

Uh oh!

JonathanWoollett-Light commented Nov 23, 2022

Uh oh!

asomers commented Nov 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanWoollett-Light commented Oct 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JonathanWoollett-Light Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveLauC commented Nov 7, 2023

Uh oh!

Uh oh!

Uh oh!

SteveLauC commented Nov 20, 2023

Uh oh!

SteveLauC commented Nov 22, 2023

Uh oh!

asomers commented Nov 23, 2023

Uh oh!

SteveLauC commented Nov 23, 2023

Uh oh!

asomers commented Nov 23, 2023

Uh oh!

SteveLauC commented Nov 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

JonathanWoollett-Light commented Nov 21, 2022 •

edited

Loading

JonathanWoollett-Light Nov 7, 2023 •

edited

Loading