Skip to content
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

Incorrect max_idle_timeout negotiation. #2008

Closed
mstyura opened this issue Oct 9, 2024 · 0 comments · Fixed by #2009
Closed

Incorrect max_idle_timeout negotiation. #2008

mstyura opened this issue Oct 9, 2024 · 0 comments · Fixed by #2009

Comments

@mstyura
Copy link
Contributor

mstyura commented Oct 9, 2024

Steps to reproduce

  1. Configure client side with custom transport parameters by overriding max_idle_timeout to Duration::ZERO
  2. Connect to quinn-based remote server which configured max_idle_timeout some some value, e.g. 180 seconds. During handshake client side will omit max_idle_timeout parameter inside TLS handshake to server which seems to correspond to this code:
    /// Milliseconds, disabled if zero
    max_idle_timeout(0x0001) = 0,

Actual result

Quinn on server side will negotiate idle_timeout to be 180 seconds because not value from client received, but client side connection will actually compute idle_timeout to be 0:

self.idle_timeout = match (self.config.max_idle_timeout, params.max_idle_timeout) {
(None, VarInt(0)) => None,
(None, x) => Some(x),
(Some(x), VarInt(0)) => Some(x),
(Some(x), y) => Some(cmp::min(x, y)),
};

effectively falling back to 3 * PTO timeout:
fn reset_idle_timeout(&mut self, now: Instant, space: SpaceId) {
let timeout = match self.idle_timeout {
None => return,
Some(x) => Duration::from_millis(x.0),
};
if self.state.is_closed() {
self.timers.stop(Timer::Idle);
return;
}
let dt = cmp::max(timeout, 3 * self.pto(space));
self.timers.set(Timer::Idle, now + dt);
}

Due to low effective idle timeout connection is frequently locally closed and need to be re-established.

Expected result

I'd expect that both sides agree on the same value of max_idle_timeout so in case local side configured with 0 timeout and peer side have non-zero idle timeout configured both endpoints will use same non-zero idle timeout.

This expectation seems to match the RFC where 0 value is threaten as disabled timeout according to section 18.2 which already respected while writing transport parameters to wire format.

The maximum idle timeout is a value in milliseconds that is encoded as an integer; see (Section 10.1). Idle timeout is disabled when both endpoints omit this transport parameter or specify a value of 0.

And according to section 10.1 during negotiation in case of only one side have enabled max_idle_timeout both endpoints must negotiate to the value provided by other side.

Each endpoint advertises a max_idle_timeout, but the effective value at an endpoint is computed as the minimum of the two advertised values (or the sole advertised value, if only one endpoint advertises a non-zero value).

@mstyura mstyura changed the title Behaviour of quinn with TransportConfig::max_idle_timeout(cfg, Some(Duration::ZERO)) Incorrect max_idle_timeout negotiation. Oct 9, 2024
mstyura added a commit to mstyura/quinn that referenced this issue Oct 9, 2024
mstyura added a commit to mstyura/quinn that referenced this issue Oct 9, 2024
mstyura added a commit to mstyura/quinn that referenced this issue Oct 9, 2024
# This is the 1st commit message:

quinn-rs#2008: Make max_idle_timeout negotiation commutative.

# This is the commit message quinn-rs#2:

quinn-rs#2008: Store negotiated idle_timeout as Duration.

# This is the commit message quinn-rs#3:

quinn-rs#2009: Address code review feedback.
mstyura added a commit to mstyura/quinn that referenced this issue Oct 9, 2024
mstyura added a commit to mstyura/quinn that referenced this issue Oct 9, 2024
Ensure that both endpoints can agree on the same idle_timeout value,
regardless of the announced max_idle_timeout parameters.
This fix addresses a corner case where a locally configured idle_timeout
of 0 was used as the negotiated value instead of the non-zero
max_idle_timeout provided by the remote side.
github-merge-queue bot pushed a commit that referenced this issue Oct 11, 2024
Ensure that both endpoints can agree on the same idle_timeout value,
regardless of the announced max_idle_timeout parameters.
This fix addresses a corner case where a locally configured idle_timeout
of 0 was used as the negotiated value instead of the non-zero
max_idle_timeout provided by the remote side.
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 a pull request may close this issue.

1 participant