Skip to content

Commit

Permalink
Fix mcap cat SIGSEGV when no schema present (#998)
Browse files Browse the repository at this point in the history
### 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`
  • Loading branch information
b-camacho authored Oct 30, 2023
1 parent c687014 commit bb31acd
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 5 deletions.
8 changes: 6 additions & 2 deletions go/cli/mcap/cmd/cat.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,10 +197,14 @@ func printMessages(
die("Failed to read next message: %s", err)
}
if !formatJSON {
schemaName := "no schema"
if schema != nil {
schemaName = schema.Name
}
if len(message.Data) > 10 {
fmt.Fprintf(w, "%d %s [%s] %v...\n", message.LogTime, channel.Topic, schema.Name, message.Data[:10])
fmt.Fprintf(w, "%d %s [%s] %v...\n", message.LogTime, channel.Topic, schemaName, message.Data[:10])
} else {
fmt.Fprintf(w, "%d %s [%s] %v\n", message.LogTime, channel.Topic, schema.Name, message.Data)
fmt.Fprintf(w, "%d %s [%s] %v\n", message.LogTime, channel.Topic, schemaName, message.Data)
}
continue
}
Expand Down
5 changes: 5 additions & 0 deletions go/cli/mcap/cmd/cat_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ func TestCat(t *testing.T) {
"../../../../tests/conformance/data/OneMessage/OneMessage-ch-chx-mx-pad-rch-rsh-st-sum.mcap",
"2 example [Example] [1 2 3]\n",
},
{
"OneSchemalessMessage",
"../../../../tests/conformance/data/OneSchemalessMessage/OneSchemalessMessage-ch-chx-mx-pad-rch-st.mcap",
"2 example [no schema] [1 2 3]\n",
},
}
for _, c := range cases {
input, err := os.ReadFile(c.inputfile)
Expand Down
9 changes: 6 additions & 3 deletions go/cli/mcap/cmd/info.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,18 +131,21 @@ func printInfo(w io.Writer, info *mcap.Info) error {
}
}

maxChanIDWidth := digits(uint64(chanIDs[len(chanIDs)-1])) + 3
maxChanIDWidth := 0
if len(chanIDs) > 0 {
maxChanIDWidth = digits(uint64(chanIDs[len(chanIDs)-1])) + 3
}
for _, chanID := range chanIDs {
channel := info.Channels[chanID]
schema := info.Schemas[channel.SchemaID]
channelMessageCount := info.Statistics.ChannelMessageCounts[chanID]
frequency := 1e9 * float64(channelMessageCount) / float64(end-start)
width := digits(uint64(chanID)) + 2
padding := strings.Repeat(" ", maxChanIDWidth-width)
row := []string{
fmt.Sprintf("\t(%d)%s%s", channel.ID, padding, channel.Topic),
}
if info.Statistics != nil {
channelMessageCount := info.Statistics.ChannelMessageCounts[chanID]
frequency := 1e9 * float64(channelMessageCount) / float64(end-start)
row = append(row, fmt.Sprintf("%*d msgs (%.2f Hz)", maxCountWidth, channelMessageCount, frequency))
}
switch {
Expand Down
41 changes: 41 additions & 0 deletions go/cli/mcap/cmd/info_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package cmd

import (
"bytes"
"os"
"path/filepath"
"strings"
"testing"

"github.com/foxglove/mcap/go/mcap"
"github.com/stretchr/testify/assert"
)

func TestInfo(t *testing.T) {
err := filepath.Walk("../../../../tests/conformance/data/", func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
return nil
}
if strings.HasSuffix(path, ".mcap") {
t.Run(path, func(t *testing.T) {
input, err := os.ReadFile(path)
assert.Nil(t, err)
r := bytes.NewReader(input)
w := new(bytes.Buffer)

reader, err := mcap.NewReader(r)
assert.Nil(t, err)
defer reader.Close()
info, err := reader.Info()
assert.Nil(t, err)
err = printInfo(w, info)
assert.Nil(t, err)
})
}
return nil
})
assert.Nil(t, err)
}

0 comments on commit bb31acd

Please sign in to comment.