Skip to content

Skip union members when discriminator is unmatched#376

Merged
Muon merged 7 commits into
mainfrom
mak/parse-valid-union-discriminator-with-no-matching-case
Apr 29, 2026
Merged

Skip union members when discriminator is unmatched#376
Muon merged 7 commits into
mainfrom
mak/parse-valid-union-discriminator-with-no-matching-case

Conversation

@Muon
Copy link
Copy Markdown
Contributor

@Muon Muon commented Apr 29, 2026

Changelog

Fix corrupted read when a union discriminator does not match any case in the union's definition.

Docs

None

Description

MessageReader silently produces corrupted data if it encounters an unknown union discriminator.

This is because it leaves the read cursor in place just after the discriminator, causing the caller to believe it finished reading the union. The caller would then start reading its next data from the middle of the union member.

In this PR, we instead skip the rest of the union if possible, or throw an error if not.

Tests have been added to cover this with mutable and final types. I've checked that they all fail without these changes.

Fixes: FG-14472

nidanin and others added 5 commits April 29, 2026 08:03
Advance past discriminator-only union values, allow discriminator-only union defaults, and cover both XCDR1 and XCDR2 mutable union reads.

Made-with: Cursor
Remove the default-value behavior change so this branch only covers advancing past discriminator-only unions that are present in encoded data.

Made-with: Cursor
@linear
Copy link
Copy Markdown

linear Bot commented Apr 29, 2026

Comment thread packages/omgidl-serialization/src/MessageReader.ts Outdated
Copy link
Copy Markdown
Contributor

@james-rms james-rms left a comment

Choose a reason for hiding this comment

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

Can you add to the description how the new test relates to the failing test data provided to us by a customer? do we know the test encodes the message in the same fashion that the user did?

Comment thread packages/omgidl-serialization/src/MessageReader.ts
@Muon
Copy link
Copy Markdown
Contributor Author

Muon commented Apr 29, 2026

I didn't derive these tests directly from the customer data. They were written to target this bug specifically.

However, I've confirmed by running the app locally that the customer's data fails to deserialize without this patch, and succeeds with it.

Although it succeeded, I do not know whether the data was deserialized correctly. Checking that by manual inspection of the message bytes would take a long time because the message is huge and complicated. (Not to mention it would be error-prone...). We can investigate further later, but I think we should ship this now because it's definitely fixing a bug and it seems to work for the customer's data.

@Muon Muon merged commit 6c2947f into main Apr 29, 2026
1 check passed
@Muon Muon deleted the mak/parse-valid-union-discriminator-with-no-matching-case branch April 29, 2026 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants