Request ID is not advanced when a read is cancelled, so the next request reuses it

[iilyak]

Crate: snmp2 0.5.2
Rust: 1.97.1
Features: default-features = false, ["tokio", "heap_buffers"]

Summary

AsyncSession advances req_id after an exchange completes. Bounding a read
with tokio::time::timeout cancels the future before that point, so the counter
is never advanced and the next request is sent with the same request ID.

The agent's late answer to the abandoned request then carries the ID the live
request is using, so validate() accepts it and the caller receives another
request's response as its own.

Root cause

src/asyncsession.rs, get_many (and get, getnext, getbulk alike):

pub async fn get_many(&mut self, oids: &[&Oid<'_>]) -> Result<Pdu<'_>> {
    self.prepare();
    let req_id = self.req_id.0;
    pdu::build_get_many(self.version, self.community.as_slice(), req_id, oids,
                        &mut self.send_pdu, ...)?;

    let resp = Pdu::from_bytes_inner(
        Self::send_and_recv(&self.socket, &self.send_pdu, &mut self.recv_buf).await?,  // <-- cancelled here
        ...
    )?;

    self.req_id += Wrapping(1);          // <-- never reached
    resp.validate(MessageType::Response, req_id, &self.community)?;
    Ok(resp)
}

send_and_recv awaits socket.recv with no timeout of its own, so a caller
that wants a bounded read has no option but to cancel the future — and doing so
skips the increment.

Reproduction

Requires an agent that answers slower than the caller waits.

// Pseudocode; the shape is what matters.
let mut session = AsyncSession::new_v2c(agent, b"public", 0).await?;

// 1. A read that gives up. The agent answers 300ms later; we wait 50ms.
let _ = tokio::time::timeout(Duration::from_millis(50), session.get_many(&batch_a)).await;

// 2. A second read. It is sent with the *same* request ID as the first.
let response = tokio::time::timeout(Duration::from_secs(1), session.get_many(&batch_b)).await??;

// 3. `response` may be the answer to batch_a: same request ID, so validate() passes.
//    Its varbinds are batch_a's OIDs, not batch_b's.

Observed in practice against a simulated agent configured to delay responses and
drop a fraction of them: a response carrying the first batch's varbinds was
accepted as the answer to the second. It surfaced only because the two batches
happened to differ in length; had they matched, the values would have been
silently attributed to the wrong objects.

Expected

Either the request ID is unique per request regardless of cancellation, or the
session is documented as not cancel-safe so callers know to discard it.

Actual

The identifier silently stops being unique at the moment of a timeout, and the
validate() check — which is otherwise exactly right — cannot distinguish the
two responses because they carry the same ID.

Suggested fix

Advance the counter before the await, so a cancelled read consumes its
identifier:

let req_id = self.req_id.0;
self.req_id += Wrapping(1);            // moved up
pdu::build_get_many(..., req_id, ...)?;
let resp = Pdu::from_bytes_inner(Self::send_and_recv(...).await?, ...)?;
resp.validate(MessageType::Response, req_id, &self.community)?;

A guard that advances on drop would work equally well. Either way the invariant
becomes "an identifier is used once", which is what validate() already assumes.

Worth noting separately: even with unique identifiers, the abandoned response
stays queued on the socket and the next read receives it first, rejects it, and
fails. Callers can work around that by replacing the session after a cancelled
read — which is what we now do — but a recv loop that skips non-matching
identifiers until its deadline would remove the need.

Impact

Silent mis-association of values between requests, on any caller that bounds
reads with a timeout — which, given the session exposes no timeout of its own, is
every caller that needs one.

How this was found

Pointing a collector at a simulated agent that delays responses past the
caller's timeout and drops a fraction of them, then observing values attributed
to objects that had not been requested.