From: Razvan Cojocaru <[email protected]> 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]> Acked-by: Arne Schwabe <[email protected]> Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1901 --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1901 This mail reflects revision 3 of this Change. Acked-by according to Gerrit (reflected above): Arne Schwabe <[email protected]> diff --git a/src/openvpn/reliable.c b/src/openvpn/reliable.c index 645fbfd..c268cce 100644 --- a/src/openvpn/reliable.c +++ b/src/openvpn/reliable.c @@ -38,8 +38,9 @@ #include "memdbg.h" -/* calculates test - base while allowing for base or test wraparound. test is - * assumed to be higher than base */ +/* calculates test - base while allowing for base or test wraparound. The + * result is only meaningful compared against a 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 +357,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 +376,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 +566,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 ac6c710..245c357 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])); } } } _______________________________________________ Openvpn-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openvpn-devel
