fix: correct FETCH object parsing of subgroup flags and object status - #35
fix: correct FETCH object parsing of subgroup flags and object status#35zafergurel wants to merge 1 commit into
Conversation
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.
lminiero
left a comment
There was a problem hiding this comment.
Thanks @zafergurel! I've added a couple of comments inline.
| return -1; /* Not enough data, try again later */ | ||
| offset += length; | ||
| } else { | ||
| } else if(subgroup_type != IMQUIC_MOQ_FETCH_SUBGROUP_ZERO) { |
There was a problem hiding this comment.
Shouldn't this be different from IMQUIC_MOQ_FETCH_SUBGROUP_ID as well? Or maybe check if it's either IMQUIC_MOQ_FETCH_SUBGROUP_PREVIOUS or IMQUIC_MOQ_FETCH_SUBGROUP_PLUS_ONE? IMQUIC_MOQ_FETCH_SUBGROUP_ID=0x03 means a Subgroup ID field is present, and so the following check on whether there are objects should not be performed (it's the only thing that would need to be done, since the if that follows does nothing for that value then) as that would fail for the first object.
| /* 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; |
There was a problem hiding this comment.
I guess that, in light of the comment above, we can remove the definition of object_status too, as it's not used for anything besides ending up in the object structure later on.
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
ifstatements, and theelsebelongs 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.