Skip to content

fix: correct FETCH object parsing of subgroup flags and object status - #35

Open
zafergurel wants to merge 1 commit into
meetecho:mainfrom
zafergurel:fix/fetch-object-serialization
Open

fix: correct FETCH object parsing of subgroup flags and object status#35
zafergurel wants to merge 1 commit into
meetecho:mainfrom
zafergurel:fix/fetch-object-serialization

Conversation

@zafergurel

Copy link
Copy Markdown
Contributor

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.

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 lminiero left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zafergurel! I've added a couple of comments inline.

Comment thread src/moq.c
return -1; /* Not enough data, try again later */
offset += length;
} else {
} else if(subgroup_type != IMQUIC_MOQ_FETCH_SUBGROUP_ZERO) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/moq.c
/* 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants