network: fix uncommitted reservation aliasing when app_io->read returns no data

fr_message_and_data_reserve() uses fr_ring_buffer_reserve() which does not
advance write_offset. If app_io->read() returns 0 (e.g. ldap_sync, which
does its own reads internally) and the cached cd is held in s->cd, any
subsequent allocation on the same message set returns the same ring-buffer
slot and zeroes it, corrupting cd->m.data.

Fix: only cache s->cd when s->leftover > 0 (partial stream data to preserve).
With no leftover, cancel the reservation explicitly via fr_message_and_data_reset()
which clears the message fields and marks the slot FR_MESSAGE_FREE so the next
reserve can reclaim it cleanly.

Also fix the B2 commit size in fr_network_read(): data_size returned by TCP
app_io already includes the leftover bytes already in the buffer, so the old
cd->m.data_size + data_size was double-counting.

diff --git a/src/lib/io/message.c b/src/lib/io/message.c
index d9b5444d2ee1bbacf1896b5614a77ccd602eefc5..702ea5edce203a9bb579a171850d1ffe077d8341 100644 (file)
--- a/src/lib/io/message.c
+++ b/src/lib/io/message.c
@@ -1009,6 +1009,26 @@ fr_message_t *fr_message_and_data_reserve(fr_message_set_t *ms, size_t reserve_s
        return message_data_reserve(ms, m, cleaned_up);
 }
 
+/** Cancel a reservation made by fr_message_and_data_reserve(), returning the slot to the set.
+ *
+ *  Both rings are uncommitted at this point (write_offset was not advanced).
+ *  Clearing the message fields is sufficient: the next reserve overwrites the
+ *  slot, and the data ring's reserved field is replaced by the new reservation size.
+ *
+ * @param[in] ms  the message set the reservation was made against.
+ * @param[in] m   the previously reserved message to cancel.
+ */
+void fr_message_and_data_reset(fr_message_set_t *ms, fr_message_t *m)
+{
+       (void) talloc_get_type_abort(ms, fr_message_set_t);
+       fr_assert(m->status == FR_MESSAGE_USED);
+       m->status = FR_MESSAGE_FREE;
+       m->rb = NULL;
+       m->data = NULL;
+       m->data_size = 0;
+       m->rb_size = 0;
+}
+
 /** Commit a previously reserved message, allocating exactly total_size bytes of packet data.
  *
  *  Commits both the message ring slot and the data ring buffer.  total_size is the

diff --git a/src/lib/io/message.h b/src/lib/io/message.h
index 687317f91bf4a3c82555972990ce1ddacf875daf..d2da6e7e2c4b8fac3fe2ebe98b05819f043d8da3 100644 (file)
--- a/src/lib/io/message.h
+++ b/src/lib/io/message.h
@@ -54,6 +54,7 @@ typedef struct {
 fr_message_set_t *fr_message_set_create(TALLOC_CTX *ctx, int num_messages, size_t message_size, size_t ring_buffer_size, bool unlimited_size) CC_HINT(nonnull);
 
 fr_message_t *fr_message_and_data_reserve(fr_message_set_t *ms, size_t reserve_size) CC_HINT(nonnull);
+void          fr_message_and_data_reset(fr_message_set_t *ms, fr_message_t *m) CC_HINT(nonnull);
 fr_message_t *fr_message_and_data_commit(fr_message_set_t *ms, fr_message_t *m, size_t total_size) CC_HINT(nonnull(1));
 fr_message_t *fr_message_and_data_alloc(fr_message_set_t *ms, size_t size) CC_HINT(nonnull);
 fr_message_t *fr_message_and_data_commit_with_leftover(fr_message_set_t *ms, fr_message_t *m, size_t actual_packet_size,

diff --git a/src/lib/io/network.c b/src/lib/io/network.c
index b4ac252e45cd45ad263f43c438a180daa0191dfc..fc0734f3de0514be252964ea3aa4db24a99fdc41 100644 (file)
--- a/src/lib/io/network.c
+++ b/src/lib/io/network.c
@@ -949,17 +949,24 @@ next_message:
                                            cd->m.data, cd->m.rb_size, &s->leftover);
        if (data_size == 0) {
                /*
-                *      Cache the message for later.  This is
-                *      important for stream sockets, which can do
-                *      partial reads into the current buffer.  We
-                *      need to be able to give the same buffer back
-                *      to the stream socket for subsequent reads.
+                *      Cache the message only when there are leftover
+                *      bytes from a partial stream read.  The buffer
+                *      must be handed back on the next call so the
+                *      stream can append to it.
                 *
-                *      Since we have a message set for each
-                *      fr_io_socket_t, no "head of line"
-                *      blocking issues can happen for stream sockets.
+                *      Without leftover bytes the reservation is
+                *      discarded; the next call makes a fresh one.
+                *      Holding an uncommitted reservation is unsafe:
+                *      any allocation on the same message set (e.g.
+                *      from a callback inside app_io->read) returns
+                *      the same ring-buffer slot and zeroes it out.
                 */
-               s->cd = cd;
+               if (s->leftover) {
+                       s->cd = cd;
+               } else {
+                       fr_message_and_data_reset(s->ms, &cd->m);
+                       s->cd = NULL;
+               }
                return;
        }