-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix mcap cat
SIGSEGV when no schema present
#998
Conversation
Hmm, how was this missed in #857? Are there more tests we need to add? |
Likely, I tried the CLI on the schemaless file that's checked in for conformance testing and found another crash
I'll throw some more tests in |
Added more tests like you suggested @jtbandes |
go/cli/mcap/cmd/info_test.go
Outdated
profile: | ||
channels: | ||
attachments: unknown | ||
metadata: unknown`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit nervous this will become annoying to update as the info command gains new functionality. This text display isn't really stable - for that we should probably introduce some kind of specification for the output.
Would it be sufficient to test that printInfo simply doesn't crash the program? If you're interested in asserting on the name of the spoofed schema, you should be able to do that by making an assertion against the info structure's Schemas field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether if we wrote that test to loop over all of the conformance files, we'd find more bugs or if this is the only one. That seems like it would be a stronger test to me, but I don't want to obligate you to address whatever else it might show.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both of these are good points!
I made the test run on every .mcap
in the conformance folder and only found one new crash, so I fixed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@b-camacho thanks for the patch and for the new test coverage! looks good to me now & I will merge it.
I will say that although we support these "schemaless" channels in the spec, it's one area I don't feel too good about. Schemaless channels can either be described "implicitly" with a zero-valued schema ID, or explicitly with a Schema record that has encoding="" and data="". IMO supporting the "implicit" version has more costs than benefits. It means all consumers need to be prepared for a nil-valued schema to appear, or else they get bugs like this, and it complicates the modeling of mcap files in a relational DB, by introducing nulls and left joins. If we instead force the writer to handle writing an empty schema, all downstream code is simplified.
Anyway, sorry for the digression... that's mostly for your curiosity & in case you didn't know of the other way. Since it's in the spec we need to support it, so thanks again for the patch.
### Public-Facing Changes `mcap cat` used to crash on some valid, but uncommon files. This fixes the crash. ### Description Mcap spec [allows for a missing schema](https://mcap.dev/spec#channel-op0x04). Currently, `mcap cat` crashes with a nil pointer dereference on such a file (unless you pass `--json`). After this change, `mcap cat` will print `[no schema]` instead of a schema for schemaless channels. Also added another test to `cat_test.go`, and added tests for `mcap info` in `info_test.go`
Public-Facing Changes
mcap cat
used to crash on some valid, but uncommon files. This fixes the crash.Description
Mcap spec allows for a missing schema.
Currently,
mcap cat
crashes with a nil pointer dereference on such a file (unless you pass--json
).After this change,
mcap cat
will print[no schema]
instead of a schema for schemaless channels.Also added another test to
cat_test.go
, and added tests formcap info
ininfo_test.go