Acked-By: Frank Lichtenheld <frank@lichtenheld.com>
Changes look good to me, applies on current master and compiles.
One error in function documentation, but probably fixable on apply. See below.
> Arne Schwabe <arne@rfc2549.org> hat am 22.04.2022 15:40 geschrieben:
[...]
> diff --git a/src/openvpn/reliable.h b/src/openvpn/reliable.h
> index cd80bbfb2..05426fd8c 100644
> --- a/src/openvpn/reliable.h
> +++ b/src/openvpn/reliable.h
> @@ -126,6 +126,28 @@ struct reliable
> bool reliable_ack_read(struct reliable_ack *ack,
> struct buffer *buf, const struct session_id *sid);
>
> +
> +/**
> + * Parse an acknowledgment record from a received packet.
> + *
> + * This function parses the packet ID acknowledgment record from the packet
> + * contained in \a buf. If the record contains acknowledgments, these are
> + * stored in \a ack. This function also compares extracts packet's session ID
Remove "compares".
> + * and returns it in \a session_id_remote
--
Frank Lichtenheld
Your patch has been applied to the master branch.
Tested on client and server ("for good measure") even if stare-at-code
suggests it's really the same code, split in half. The change in
ssl.c is due to "ack->len is now always zeroed".
Uncrustify (0.74.0_f) has opinions on "if ( a && (b || c))"
constructs, so a bit of whitespace was changed.
Comment in reliable.h adjusted ("compares extracts").
commit ac97e16123aba7d4792ca978a4e2e6875973b8ed
Author: Arne Schwabe
Date: Fri Apr 22 15:40:37 2022 +0200
Split out reliable_ack_parse from reliable_ack_read
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
Message-Id: <20220422134038.3801239-9-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24145.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
@@ -153,56 +153,64 @@ reliable_ack_acknowledge_packet_id(struct reliable_ack *ack, packet_id_type pid)
return false;
}
-/* read a packet ID acknowledgement record from buf into ack */
+
bool
reliable_ack_read(struct reliable_ack *ack,
struct buffer *buf, const struct session_id *sid)
{
- struct gc_arena gc = gc_new();
- int i;
- uint8_t count;
- packet_id_type net_pid;
- packet_id_type pid;
struct session_id session_id_remote;
+ if (!reliable_ack_parse(buf, ack, &session_id_remote))
+ {
+ return false;
+ }
+
+ if (ack->len >= 1 && (!session_id_defined(&session_id_remote)
+ || !session_id_equal(&session_id_remote, sid)))
+ {
+ struct gc_arena gc = gc_new();
+ dmsg(D_REL_LOW,
+ "ACK read BAD SESSION-ID FROM REMOTE, local=%s, remote=%s",
+ session_id_print(sid, &gc), session_id_print(&session_id_remote, &gc));
+ gc_free(&gc);
+ return false;
+ }
+ return true;
+}
+
+bool
+reliable_ack_parse(struct buffer *buf, struct reliable_ack *ack,
+ struct session_id *session_id_remote)
+{
+ uint8_t count;
+ ack->len = 0;
+
if (!buf_read(buf, &count, sizeof(count)))
{
- goto error;
+ return false;
}
- for (i = 0; i < count; ++i)
+ for (int i = 0; i < count; ++i)
{
+ packet_id_type net_pid;
if (!buf_read(buf, &net_pid, sizeof(net_pid)))
{
- goto error;
+ return false;
}
if (ack->len >= RELIABLE_ACK_SIZE)
{
- goto error;
+ return false;
}
- pid = ntohpid(net_pid);
+ packet_id_type pid = ntohpid(net_pid);
ack->packet_id[ack->len++] = pid;
}
if (count)
{
- if (!session_id_read(&session_id_remote, buf))
- {
- goto error;
- }
- if (!session_id_defined(&session_id_remote)
- || !session_id_equal(&session_id_remote, sid))
+ if (!session_id_read(session_id_remote, buf))
{
- dmsg(D_REL_LOW,
- "ACK read BAD SESSION-ID FROM REMOTE, local=%s, remote=%s",
- session_id_print(sid, &gc), session_id_print(&session_id_remote, &gc));
- goto error;
+ return false;
}
}
- gc_free(&gc);
return true;
-
-error:
- gc_free(&gc);
- return false;
}
/* write a packet ID acknowledgement record to buf, */
@@ -126,6 +126,28 @@ struct reliable
bool reliable_ack_read(struct reliable_ack *ack,
struct buffer *buf, const struct session_id *sid);
+
+/**
+ * Parse an acknowledgment record from a received packet.
+ *
+ * This function parses the packet ID acknowledgment record from the packet
+ * contained in \a buf. If the record contains acknowledgments, these are
+ * stored in \a ack. This function also compares extracts packet's session ID
+ * and returns it in \a session_id_remote
+ *
+ * @param ack The acknowledgment structure in which received
+ * acknowledgments are to be stored.
+ * @param buf The buffer containing the packet.
+ * @param session_id_remote The parsed remote session id. This field is
+ * is only filled if ack->len >= 1
+ * @return
+ * @li True, if processing was successful.
+ * @li False, if an error occurs during processing.
+ */
+bool
+reliable_ack_parse(struct buffer *buf, struct reliable_ack *ack,
+ struct session_id *session_id_remote);
+
/**
* Remove acknowledged packets from a reliable structure.
*
@@ -3435,7 +3435,6 @@ tls_pre_decrypt(struct tls_multi *multi,
/* buffers all packet IDs to delete from send_reliable */
struct reliable_ack send_ack;
- send_ack.len = 0;
if (!reliable_ack_read(&send_ack, buf, &session->session_id))
{
msg(D_TLS_ERRORS,