[M] Change in openvpn[master]: reliable: fix the send window width
"razvanc \(Code Review\) via Openvpn-devel" <[email protected]>
| Newsgroups | gmane.network.openvpn.devel |
|---|---|
| Message-ID | <[email protected]> |
Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1901?usp=email
to review the following change.
Change subject: reliable: fix the send window width
......................................................................
reliable: fix the send window width
The check accepted the 11 ids below rel->packet_id where its documentation
said RELIABLE_CAPACITY. Twelve is right: reliable_get_buf_output_sequenced()
issues an id while it is less than rel->size beyond the oldest unacknowledged
one, so an entry can sit a full rel->size below rel->packet_id, and rel->size
may be RELIABLE_CAPACITY.
No effect today, TLS_RELIABLE_N_SEND_BUFFERS being 6. Raising the send buffer
count to RELIABLE_CAPACITY, which reliable_init() permits, would have
discarded the ACK for the oldest outstanding packet.
The width is now the extent passed to reliable_pid_in_range() rather than a
comparison operator.
Also renames validate_packet_id_window() to reliable_pid_in_send_window(),
returning bool and taking a const struct reliable * like its neighbours, and
the same const on reliable_get_num_output_sequenced_available().
Three doc comments described what the code does not do: the window bounds,
reliable_wont_break_sequentiality()'s reference id, subtract_pid()'s argument
order. reliable_mark_active_incoming() now states the precondition it asserts.
Change-Id: I63c141075d89ae6a07872e784faff817dd3350b1
Signed-off-by: Razvan Cojocaru <[email protected]>
---
M src/openvpn/reliable.c
M src/openvpn/reliable.h
M tests/unit_tests/openvpn/test_packet_id.c
3 files changed, 67 insertions(+), 54 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/01/1901/1
diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c
index 6e10211..7ddc03a 100644
--- a/src/openvpn/reliable.c
+++ b/src/openvpn/reliable.c
@@ -38,8 +38,8 @@
#include "memdbg.h"
-/* calculates test - base while allowing for base or test wraparound. test is
- * assumed to be higher than base */
+/* test - base in the packet id space, wrapping around. Only meaningful against
+ * an explicit bound: on its own it does not say which id came first. */
static inline packet_id_type
subtract_pid(const packet_id_type test, const packet_id_type base)
{
@@ -356,12 +356,11 @@
return true;
}
-int
-validate_packet_id_window(struct reliable *rel, packet_id_type pid)
+bool
+reliable_pid_in_send_window(const struct reliable *rel, packet_id_type pid)
{
- const packet_id_type dist = subtract_pid(rel->packet_id, pid);
-
- return dist > 0 && dist < RELIABLE_CAPACITY;
+ return reliable_pid_in_range(pid, subtract_pid(rel->packet_id, RELIABLE_CAPACITY),
+ RELIABLE_CAPACITY);
}
/* del acknowledged items from send buf */
@@ -376,7 +375,7 @@
packet_id_type pid = ack->packet_id[i];
- if (!validate_packet_id_window(rel, pid))
+ if (!reliable_pid_in_send_window(rel, pid))
{
if (out_of_window == 0)
{
@@ -566,7 +565,7 @@
}
int
-reliable_get_num_output_sequenced_available(struct reliable *rel)
+reliable_get_num_output_sequenced_available(const struct reliable *rel)
{
return rel->size - (int)reliable_oldest_active_distance(rel);
}
diff --git a/src/openvpn/reliable.h b/src/openvpn/reliable.h
index 04f3372..534feb3 100644
--- a/src/openvpn/reliable.h
+++ b/src/openvpn/reliable.h
@@ -190,15 +190,20 @@
}
/**
- * check that pid is inside the window of possible outstanding packets
- * of size RELIABLE_CAPACITY, ie inside the range
- * [rel->packet_id - RELIABLE_CAPACITY, rel->packet_id).
+ * Check whether pid is one of the RELIABLE_CAPACITY ids below rel->packet_id,
+ * and so could still be outstanding. rel->packet_id is the *next* id to be
+ * sent, so it is not itself in the window.
*
- * rel->packet is the *next* packet id to be sent out, so it is not
- * included in the valid range.
+ * pid comes off the wire; all 2^32 values are handled.
+ *
+ * @param rel The reliable structure holding this tunnel's sent packets.
+ * @param pid A packet ID from a received acknowledgment.
+ *
+ * @return
+ * @li True, if pid is inside the window.
+ * @li False, otherwise.
*/
-int
-validate_packet_id_window(struct reliable *rel, packet_id_type pid);
+bool reliable_pid_in_send_window(const struct reliable *rel, packet_id_type pid);
/**
* Returns the number of packets that need to be acked.
@@ -305,14 +310,15 @@
* Check that a received packet's ID can safely be stored in
* the reliable structure's processing window.
*
- * This function checks the difference between the received packet's ID
- * and the lowest non-acknowledged packet ID in the given reliable
- * structure. If that difference is larger than the total number of
- * packets which can be stored, then this packet cannot be stored safely,
- * because the reliable structure could possibly fill up without leaving
- * room for all intervening packets. In that case, this received packet
- * could break the reliable structure's sequentiality, and must therefore
- * be discarded.
+ * Checks the received packet's ID against rel->packet_id, the next ID the
+ * reliability layer expects to hand upwards. One too far ahead could fill the
+ * structure without leaving room for the intervening packets, so it is
+ * discarded.
+ *
+ * IDs numerically below rel->packet_id are accepted here, an absolute
+ * comparison that an ID from before a wraparound does not pass.
+ * reliable_not_replay() rejects them, and they are still acknowledged so a
+ * peer whose ACK was lost stops retransmitting.
*
* @param rel The reliable structure for handling this VPN tunnel's
* received packets.
@@ -354,6 +360,10 @@
* Mark the %reliable entry associated with the given buffer as active
* incoming.
*
+ * pid must be less than rel->size ahead of rel->packet_id. This is asserted,
+ * so run reliable_wont_break_sequentiality() and reliable_not_replay()
+ * first.
+ *
* @param rel The reliable structure associated with this packet.
* @param buf The buffer into which the packet has been copied.
* @param pid The packet's packet ID.
@@ -446,7 +456,7 @@
* @return the number of buffer that are available for sending without
* breaking ack sequence
* */
-int reliable_get_num_output_sequenced_available(struct reliable *rel);
+int reliable_get_num_output_sequenced_available(const struct reliable *rel);
/**
* Mark the reliable entry associated with the given buffer as
diff --git a/tests/unit_tests/openvpn/test_packet_id.c b/tests/unit_tests/openvpn/test_packet_id.c
index 7df74d9..e101412 100644
--- a/tests/unit_tests/openvpn/test_packet_id.c
+++ b/tests/unit_tests/openvpn/test_packet_id.c
@@ -331,44 +331,50 @@
struct reliable rel = { 0 };
rel.packet_id = 1;
- assert_true(validate_packet_id_window(&rel, 0));
+ assert_true(reliable_pid_in_send_window(&rel, 0));
/* packet id 1 is outside the window as it is the *next* packet id */
- assert_false(validate_packet_id_window(&rel, 1));
+ assert_false(reliable_pid_in_send_window(&rel, 1));
/* wrapped around packet id, "-2" */
- assert_true(validate_packet_id_window(&rel, 0xFFFFFFFD));
+ assert_true(reliable_pid_in_send_window(&rel, 0xFFFFFFFD));
/* wrapped around packet id, "-10" */
- assert_true(validate_packet_id_window(&rel, 0xFFFFFFF6));
+ assert_true(reliable_pid_in_send_window(&rel, 0xFFFFFFF6));
- /* wrapped around packet id, "-11" */
- assert_false(validate_packet_id_window(&rel, 0xFFFFFFF5));
- assert_false(validate_packet_id_window(&rel, 0x80000000));
+ /* wrapped around packet id, "-11": the oldest id still in the window */
+ assert_true(reliable_pid_in_send_window(&rel, 0xFFFFFFF5));
+
+ /* one further back is outside it */
+ assert_false(reliable_pid_in_send_window(&rel, 0xFFFFFFF4));
+ assert_false(reliable_pid_in_send_window(&rel, 0x80000000));
rel.packet_id = 0x80000000;
/* near the signed/usigned integer area */
- assert_false(validate_packet_id_window(&rel, 0x80000001));
- assert_true(validate_packet_id_window(&rel, 0x7fffffff));
- assert_true(validate_packet_id_window(&rel, 0x7ffffff5));
- assert_false(validate_packet_id_window(&rel, 0x7ffffff4));
+ assert_false(reliable_pid_in_send_window(&rel, 0x80000001));
+ assert_true(reliable_pid_in_send_window(&rel, 0x7fffffff));
+ assert_true(reliable_pid_in_send_window(&rel, 0x7ffffff5));
+ assert_true(reliable_pid_in_send_window(&rel, 0x7ffffff4));
+ assert_false(reliable_pid_in_send_window(&rel, 0x7ffffff3));
rel.packet_id = 0xFFFFFFFD;
- assert_false(validate_packet_id_window(&rel, 0xFFFFFFFD));
- assert_false(validate_packet_id_window(&rel, 0));
- assert_false(validate_packet_id_window(&rel, 1));
- assert_false(validate_packet_id_window(&rel, 0xFFFFFFFE));
- assert_false(validate_packet_id_window(&rel, 0xFFFFFFFF));
- assert_true(validate_packet_id_window(&rel, 0xFFFFFFF3));
- assert_true(validate_packet_id_window(&rel, 0xFFFFFFF2));
- assert_false(validate_packet_id_window(&rel, 0xFFFFFFF1));
+ assert_false(reliable_pid_in_send_window(&rel, 0xFFFFFFFD));
+ assert_false(reliable_pid_in_send_window(&rel, 0));
+ assert_false(reliable_pid_in_send_window(&rel, 1));
+ assert_false(reliable_pid_in_send_window(&rel, 0xFFFFFFFE));
+ assert_false(reliable_pid_in_send_window(&rel, 0xFFFFFFFF));
+ assert_true(reliable_pid_in_send_window(&rel, 0xFFFFFFF3));
+ assert_true(reliable_pid_in_send_window(&rel, 0xFFFFFFF2));
+ assert_true(reliable_pid_in_send_window(&rel, 0xFFFFFFF1));
+ assert_false(reliable_pid_in_send_window(&rel, 0xFFFFFFF0));
rel.packet_id = 500;
- assert_false(validate_packet_id_window(&rel, 501));
- assert_true(validate_packet_id_window(&rel, 497));
- assert_true(validate_packet_id_window(&rel, 500 - (RELIABLE_CAPACITY - 1)));
- assert_false(validate_packet_id_window(&rel, 500 - RELIABLE_CAPACITY));
+ assert_false(reliable_pid_in_send_window(&rel, 501));
+ assert_true(reliable_pid_in_send_window(&rel, 497));
+ assert_true(reliable_pid_in_send_window(&rel, 500 - (RELIABLE_CAPACITY - 1)));
+ assert_true(reliable_pid_in_send_window(&rel, 500 - RELIABLE_CAPACITY));
+ assert_false(reliable_pid_in_send_window(&rel, 500 - (RELIABLE_CAPACITY + 1)));
}
@@ -533,21 +539,19 @@
return (packet_id_type)(p1 - p2) >= 0x80000000u;
}
-/* one short of RELIABLE_CAPACITY, which is what the code accepts today */
-#define CHARACTERIZED_SEND_WINDOW 11
-
/* TLS_RELIABLE_N_SEND_BUFFERS and P_CONTROL_V1, from ssl_pkt.h, which this
* test binary does not pull in */
#define CHAR_N_SEND_BUFFERS 6
#define CHAR_OPCODE_CONTROL_V1 4
-/* preserved: the ids validate_packet_id_window() accepts */
+/* introduced: the RELIABLE_CAPACITY ids below rel->packet_id.
+ * validate_packet_id_window() stopped one short of that. */
static bool
ref_pid_in_send_window(const struct reliable *rel, packet_id_type pid)
{
const packet_id_type dist = (packet_id_type)(rel->packet_id - pid);
- return dist >= 1 && dist <= CHARACTERIZED_SEND_WINDOW;
+ return dist >= 1 && dist <= RELIABLE_CAPACITY;
}
/* preserved: reliable_pid_in_range2() */
@@ -798,7 +802,7 @@
rel.packet_id = anchors[a];
assert_int_equal(ref_pid_in_send_window(&rel, ids[i]),
- validate_packet_id_window(&rel, ids[i]) != 0);
+ reliable_pid_in_send_window(&rel, ids[i]));
}
}
}
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1901?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I63c141075d89ae6a07872e784faff817dd3350b1
Gerrit-Change-Number: 1901
Gerrit-PatchSet: 1
Gerrit-Owner: razvanc <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel