2024-11-14, 09:12:01 +0100, Antonio Quartulli wrote: > On 13/11/2024 11:36, Sabrina Dubroca wrote: > > 2024-11-12, 14:20:45 +0100, Antonio Quartulli wrote: > > > On 05/11/2024 19:10, Sabrina Dubroca wrote: > > > > 2024-10-29, 11:47:28 +0100, Antonio Quartulli wrote: > > > > > + /* check for peer timeout */ > > > > > + expired = false; > > > > > + timeout = peer->keepalive_timeout; > > > > > + delta = now - peer->last_recv; > > > > > > > > I'm not sure that's always > 0 if we finish decrypting a packet just > > > > as the workqueue starts: > > > > > > > > ovpn_peer_keepalive_work > > > > now = ... > > > > > > > > ovpn_decrypt_post > > > > peer->last_recv = ... > > > > > > > > ovpn_peer_keepalive_work_single > > > > delta: now < peer->last_recv > > > > > > > > > > Yeah, there is nothing preventing this from happening...but is this truly > > > a > > > problem? The math should still work, no? > > > > We'll fail "delta < timeout" (which we shouldn't), so we'll end up > > either in the "expired = true" case, or not updating > > keepalive_recv_exp. Both of these seem not ideal. > > delta is signed, so it'll end up being a negative value and "delta < > timeout" should not fail then. Unless I am missing something.
But timeout is "unsigned long", so the comparison will be done as unsigned. > Anyway, this was just an exercise to understand what was going on. > I already changed the code as per your suggestion (the fact that we are > still discussing this chunk proves that it needed to be simplified :)) :) -- Sabrina
