From db878520710e6844cccc5f28fc80c56840ca4ae8 Mon Sep 17 00:00:00 2001 From: Zafer Gurel <1393083+zafergurel@users.noreply.github.com> Date: Tue, 11 Aug 2026 18:47:59 +0300 Subject: [PATCH 1/2] fix: correct FETCH object parsing of subgroup flags and object status Three defects in the FETCH object path, found while testing against another draft-18 implementation. All three are in how a FETCH object is read, so a peer that serialises it correctly is misparsed. Subgroup serialization flags: the four branches selecting the subgroup mode are independent `if` statements, and the `else` belongs to the third alone. Modes 0x00 and 0x01 are therefore assigned and then immediately overwritten with SUBGROUP_ID, so the parser reads a Subgroup ID field that the sender never wrote and every field after it shifts by one varint. Only 0x02 and 0x03 came out right. Turning the chain into if/else if fixes it. Subgroup ID of zero: with the chain corrected, mode 0x00 reached the branch that resolves a subgroup from the previous object, which rejects the first object on a stream as referencing a non-existing prior object. A Subgroup ID of zero is a literal, not a back-reference, so it is now excluded from that branch and the id keeps its initial value. Object status: a FETCH object has no Object Status field. Both the parser and the serialiser treated a zero length payload as introducing one, which is the Subgroup object layout, not the Fetch one -- Figure 27 has no status field, and neither did the equivalent figure in draft-16, so this is not version specific. A missing or unknown range is conveyed with an End of Range indicator instead. Since both sides agreed, imquic interoperated with itself and only failed against other implementations. Also corrects two `if(*ptr)` null checks that dereference the value instead of testing the pointer, one of which meant a protocol violation was only ever reported if the flag was already set. Verified against a MOQtail relay: a FETCH of 89 cached objects previously desynchronised at the first object and delivered garbage subgroup ids, zero length payloads and broken properties; it now arrives intact. The status change was confirmed separately by serving a zero length object and observing the stream truncate at that point without the fix and complete with it. --- src/moq.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/src/moq.c b/src/moq.c index 7d33771..b0e1ffe 100644 --- a/src/moq.c +++ b/src/moq.c @@ -930,7 +930,7 @@ void imquic_moq_parse_fetch_serialization_flags(imquic_moq_version version, uint gboolean *datagram, gboolean *end_ne_range, gboolean *end_uk_range, gboolean *violation) { /* Make sure the provided flags are valid, or return a protocol violation */ if(!imquic_moq_is_fetch_serialization_flags_valid(version, flags)) { - if(*violation) + if(violation) *violation = TRUE; return; } @@ -944,12 +944,12 @@ void imquic_moq_parse_fetch_serialization_flags(imquic_moq_version version, uint /* If we're here, we're parsing a bitmask of a single byte */ uint8_t flags8 = (uint8_t)flags; uint8_t lsb = flags8 & 0x03; - if(*subgroup) { + if(subgroup) { if(lsb == 0x00) *subgroup = IMQUIC_MOQ_FETCH_SUBGROUP_ZERO; - if(lsb == 0x01) + else if(lsb == 0x01) *subgroup = IMQUIC_MOQ_FETCH_SUBGROUP_PREVIOUS; - if(lsb == 0x02) + else if(lsb == 0x02) *subgroup = IMQUIC_MOQ_FETCH_SUBGROUP_PLUS_ONE; else *subgroup = IMQUIC_MOQ_FETCH_SUBGROUP_ID; @@ -4765,7 +4765,7 @@ int imquic_moq_parse_fetch_header_object(imquic_moq_context *moq, imquic_moq_str if(length == 0 || length >= blen-offset) return -1; /* Not enough data, try again later */ offset += length; - } else { + } else if(subgroup_type != IMQUIC_MOQ_FETCH_SUBGROUP_ZERO) { /* The subgroup ID references a previous object */ IMQUIC_MOQ_CHECK_ERR(!moq_stream->got_objects, error, IMQUIC_MOQ_PROTOCOL_VIOLATION, -1, "Serialization flag references non-existing previous object"); if(subgroup_type == IMQUIC_MOQ_FETCH_SUBGROUP_PREVIOUS) @@ -4814,16 +4814,10 @@ int imquic_moq_parse_fetch_header_object(imquic_moq_context *moq, imquic_moq_str if(length == 0 || length >= blen-offset) return -1; /* Not enough data, try again later */ offset += length; + /* A FETCH object has no Object Status field: a zero length payload is a zero + * length object, and a range that is missing or unknown is conveyed with an + * End of Range indicator instead. Only Subgroup objects carry the status. */ uint64_t object_status = 0; - if(p_len == 0) { - object_status = imquic_read_moqint(moq->version, &bytes[offset], blen-offset, &length); - if(length == 0 || length > blen-offset) - return -1; /* Not enough data, try again later */ - /* TODO An invalid object status should be a protocol violation error */ - //~ IMQUIC_MOQ_CHECK_ERR(object_status > IMQUIC_MOQ_END_OF_TRACK, error, IMQUIC_MOQ_PROTOCOL_VIOLATION, 0, "Invalid object status"); - //~ IMQUIC_MOQ_CHECK_ERR(object_status == IMQUIC_MOQ_OBJECT_DOESNT_EXIST && prop_len > 0, error, IMQUIC_MOQ_PROTOCOL_VIOLATION, 0, "Properties received in object with status 'Does Not Exist'"); - offset += length; - } if(p_len > blen-offset) return -1; /* Not enough data, try again later */ IMQUIC_LOG(IMQUIC_MOQ_LOG_HUGE, "[%s][MoQ] -- Group ID: %"SCNu64"\n", @@ -5994,8 +5988,6 @@ size_t imquic_moq_add_fetch_header_object(imquic_moq_context *moq, uint8_t *byte if(payload == NULL) plen = 0; offset += imquic_write_moqint(moq->version, pplen + plen, &bytes[offset], blen-offset); - if(plen == 0 && pplen == 0) - offset += imquic_write_moqint(moq->version, object_status, &bytes[offset], blen-offset); if(pplen > 0) { memcpy(&bytes[offset], payload_prefix, pplen); offset += pplen; From 4d775410f9f2caec21d22751dd84561ded045976 Mon Sep 17 00:00:00 2001 From: Zafer Gurel <1393083+zafergurel@users.noreply.github.com> Date: Mon, 31 Aug 2026 13:32:13 +0300 Subject: [PATCH 2/2] fix: address review on FETCH subgroup back-references Name the two modes the previous-object branch handles instead of excluding one, and resolve against last_subgroup_id, which the FETCH parser actually maintains, rather than subgroup_id, which stays zero on a FETCH stream. Drop the object_status local and the serialiser parameter left dead once the status write went away. --- src/internal/moq.h | 3 +-- src/moq.c | 21 ++++++++------------- 2 files changed, 9 insertions(+), 15 deletions(-) diff --git a/src/internal/moq.h b/src/internal/moq.h index d6866c9..a936bd8 100644 --- a/src/internal/moq.h +++ b/src/internal/moq.h @@ -1171,7 +1171,6 @@ size_t imquic_moq_add_fetch_header(imquic_moq_context *moq, uint8_t *bytes, size * @param subgroup_id The subgroup ID * @param object_id The object ID * @param priority The publisher priority to put in the message - * @param object_status The object status (only added if the payload length is 0) * @param payload_prefix The buffer containing the payload prefix of the object, if needed * @param pplen The size of the payload prefix buffer * @param payload The buffer containing the payload of the object @@ -1181,7 +1180,7 @@ size_t imquic_moq_add_fetch_header(imquic_moq_context *moq, uint8_t *bytes, size * @returns The size of the generated object, if successful, or 0 otherwise */ size_t imquic_moq_add_fetch_header_object(imquic_moq_context *moq, uint8_t *bytes, size_t blen, uint64_t flags, uint64_t group_id, uint64_t subgroup_id, uint64_t object_id, uint8_t priority, - uint64_t object_status, uint8_t *payload_prefix, size_t pplen, uint8_t *payload, size_t plen, uint8_t *properties, size_t prlen); + uint8_t *payload_prefix, size_t pplen, uint8_t *payload, size_t plen, uint8_t *properties, size_t prlen); /*! \brief Helper to add padding data to a buffer, formatted as expected * for \c PADDING_STREAM or \c PADDING_DATAGRAM * @param moq The imquic_moq_context generating the object diff --git a/src/moq.c b/src/moq.c index b0e1ffe..be8f3cf 100644 --- a/src/moq.c +++ b/src/moq.c @@ -4765,13 +4765,13 @@ int imquic_moq_parse_fetch_header_object(imquic_moq_context *moq, imquic_moq_str if(length == 0 || length >= blen-offset) return -1; /* Not enough data, try again later */ offset += length; - } else if(subgroup_type != IMQUIC_MOQ_FETCH_SUBGROUP_ZERO) { + } else if(subgroup_type == IMQUIC_MOQ_FETCH_SUBGROUP_PREVIOUS || + subgroup_type == IMQUIC_MOQ_FETCH_SUBGROUP_PLUS_ONE) { /* The subgroup ID references a previous object */ IMQUIC_MOQ_CHECK_ERR(!moq_stream->got_objects, error, IMQUIC_MOQ_PROTOCOL_VIOLATION, -1, "Serialization flag references non-existing previous object"); - if(subgroup_type == IMQUIC_MOQ_FETCH_SUBGROUP_PREVIOUS) - subgroup_id = moq_stream->subgroup_id; - else if(subgroup_type == IMQUIC_MOQ_FETCH_SUBGROUP_PLUS_ONE) - subgroup_id = moq_stream->subgroup_id + 1; + subgroup_id = moq_stream->last_subgroup_id; + if(subgroup_type == IMQUIC_MOQ_FETCH_SUBGROUP_PLUS_ONE) + subgroup_id++; } uint64_t object_id = 0; if(has_oid) { @@ -4817,7 +4817,6 @@ int imquic_moq_parse_fetch_header_object(imquic_moq_context *moq, imquic_moq_str /* A FETCH object has no Object Status field: a zero length payload is a zero * length object, and a range that is missing or unknown is conveyed with an * End of Range indicator instead. Only Subgroup objects carry the status. */ - uint64_t object_status = 0; if(p_len > blen-offset) return -1; /* Not enough data, try again later */ IMQUIC_LOG(IMQUIC_MOQ_LOG_HUGE, "[%s][MoQ] -- Group ID: %"SCNu64"\n", @@ -4828,10 +4827,6 @@ int imquic_moq_parse_fetch_header_object(imquic_moq_context *moq, imquic_moq_str imquic_get_connection_name(moq->conn), object_id); IMQUIC_LOG(IMQUIC_MOQ_LOG_HUGE, "[%s][MoQ] -- Payload Length: %"SCNu64"\n", imquic_get_connection_name(moq->conn), p_len); - if(p_len == 0) { - IMQUIC_LOG(IMQUIC_MOQ_LOG_HUGE, "[%s][MoQ] -- Object Status: %"SCNu64"\n", - imquic_get_connection_name(moq->conn), object_status); - } if(!moq_stream->got_objects) moq_stream->got_objects = TRUE; moq_stream->last_group_id = group_id; @@ -4850,7 +4845,7 @@ int imquic_moq_parse_fetch_header_object(imquic_moq_context *moq, imquic_moq_str .group_id = group_id, .subgroup_id = subgroup_id, .object_id = object_id, - .object_status = object_status, + .object_status = IMQUIC_MOQ_NORMAL_OBJECT, .priority = priority, .payload = bytes + offset, .payload_len = p_len, @@ -5959,7 +5954,7 @@ size_t imquic_moq_add_fetch_header(imquic_moq_context *moq, uint8_t *bytes, size size_t imquic_moq_add_fetch_header_object(imquic_moq_context *moq, uint8_t *bytes, size_t blen, uint64_t flags, uint64_t group_id, uint64_t subgroup_id, uint64_t object_id, uint8_t priority, - uint64_t object_status, uint8_t *payload_prefix, size_t pplen, uint8_t *payload, size_t plen, uint8_t *properties, size_t prlen) { + uint8_t *payload_prefix, size_t pplen, uint8_t *payload, size_t plen, uint8_t *properties, size_t prlen) { if(bytes == NULL || blen < 1) { IMQUIC_LOG(IMQUIC_LOG_ERR, "[%s][MoQ] Can't add MoQ %s object: invalid arguments\n", imquic_get_connection_name(moq->conn), imquic_moq_data_message_type_str(IMQUIC_MOQ_FETCH_HEADER, moq->version)); @@ -8741,7 +8736,7 @@ int imquic_moq_send_object(imquic_connection *conn, imquic_moq_object *object) { moq_stream->last_group_id = object->group_id; moq_stream->last_object_id = object->object_id; shto_len = imquic_moq_add_fetch_header_object(moq, buffer, bufsize, flags, - group_id, object->subgroup_id, object_id, object->priority, object->object_status, + group_id, object->subgroup_id, object_id, object->priority, object->payload_prefix, object->payload_prefix_len, object->payload, object->payload_len, properties, properties_len);