Usually we use the SEQ_{LT,LE,GT,GE}() comparison macros on raw sequence
numbers, to see how they related handling possible wraparounds. However,
in two cases we take a difference of sequence numbers then compare that
difference to 0.
As long as the difference is done using wrapping unsigned arithmetic,
that's not incorrect. It's essentially doing comparisons in a "shifted"
space of sequence numbers, and I find working in both the raw and shifted
spaces makes it harder to reason about. Specifically I sometimes find I
have to go through an additional layer of logic to convince myself that the
wraparound handling is correct.
In addition to confusing me, it also tends to confuse cppcheck, at least in
cppcheck 2.21.0 it often seems to trip false positives via cppcheck bug
14848.
It turns out that in each of the cases we use comparisons of sequence
differences against 0, it's pretty much just as clear - maybe clearer to
perform the comparisons against raw sequence numbers. We still need the
difference afterwards, but it's much more obvious that it means what we
need it do, if we've already eliminated cases where the sequence numbers
aren't in the expected order.
In the case in tcp_data_from_tap() this also lets us simplify the diagram
explanation of the different cases a bit. While there we correct the
diagram in the second discard case: seq was shown as partway into the
packet, which is not true by definition. Instead seq_from_tap should be
some distance after the end of the packet.
Link: https://trac.cppcheck.net/ticket/14848
Signed-off-by: David Gibson
---
tcp.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/tcp.c b/tcp.c
index dcadc765..edcabe27 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1840,21 +1840,20 @@ static int tcp_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn)
uint32_t wnd_scaled = conn->wnd_from_tap << conn->ws_from_tap;
uint32_t already_sent;
- /* How much have we read/sent since last received ack ? */
- already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
-
- if (SEQ_LT(already_sent, 0)) {
+ if (SEQ_LT(conn->seq_to_tap, conn->seq_ack_from_tap)) {
/* RFC 761, section 2.1. */
flow_trace(conn, "ACK sequence gap: ACK for %u, sent: %u",
conn->seq_ack_from_tap, conn->seq_to_tap);
conn->seq_to_tap = conn->seq_ack_from_tap;
- already_sent = 0;
if (tcp_set_peek_offset(conn, 0)) {
tcp_rst(c, conn);
return -1;
}
}
+ /* How much have we read/sent since last received ack ? */
+ already_sent = conn->seq_to_tap - conn->seq_ack_from_tap;
+
if (!wnd_scaled || already_sent >= wnd_scaled) {
conn_flag(c, conn, ACK_FROM_TAP_BLOCKS);
conn_flag(c, conn, STALLED);
@@ -1997,37 +1996,34 @@ static int tcp_data_from_tap(const struct ctx *c, struct tcp_tap_conn *conn,
if (!len)
continue;
- seq_offset = seq_from_tap - seq;
/* Use data from this buffer only in these two cases:
*
* , seq_from_tap , seq_from_tap
* |--------| <-- len |--------| <-- len
- * '----' <-- offset ' <-- offset
* ^ seq ^ seq
- * (offset >= 0, seq + len > seq_from_tap)
+ * (seq_from_tap >= seq, seq + len > seq_from_tap)
*
* discard in these two cases:
- * , seq_from_tap , seq_from_tap
+ * , seq_from_tap , seq_from_tap
* |--------| <-- len |--------| <-- len
- * '--------' <-- offset '-----| <- offset
- * ^ seq ^ seq
- * (offset >= 0, seq + len <= seq_from_tap)
+ * ^ seq ^ seq
+ * (seq_from_tap >= seq, seq + len <= seq_from_tap)
*
* keep, look for another buffer, then go back, in this case:
* , seq_from_tap
* |--------| <-- len
- * '===' <-- offset
* ^ seq
- * (offset < 0)
+ * (seq_from_tap < seq)
*/
- if (SEQ_GE(seq_offset, 0) && SEQ_LE(seq + len, seq_from_tap))
+ if (SEQ_GE(seq_from_tap, seq) && SEQ_LE(seq + len, seq_from_tap))
continue;
- if (SEQ_LT(seq_offset, 0)) {
+ if (SEQ_LT(seq_from_tap, seq)) {
if (keep == -1)
keep = i;
continue;
}
+ seq_offset = seq_from_tap - seq;
iov_drop_header(&data, seq_offset);
size = len - seq_offset;
--
2.54.0