From bb31acdb277f1cfaa67c4dfae11a552dd00c759d Mon Sep 17 00:00:00 2001
From: Brian Camacho <hello@briancamacho.me>
Date: Mon, 30 Oct 2023 09:59:14 -0700
Subject: [PATCH] Fix `mcap cat` SIGSEGV when no schema present (#998)

### 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`
---
 go/cli/mcap/cmd/cat.go       |  8 +++++--
 go/cli/mcap/cmd/cat_test.go  |  5 +++++
 go/cli/mcap/cmd/info.go      |  9 +++++---
 go/cli/mcap/cmd/info_test.go | 41 ++++++++++++++++++++++++++++++++++++
 4 files changed, 58 insertions(+), 5 deletions(-)
 create mode 100644 go/cli/mcap/cmd/info_test.go

diff --git a/go/cli/mcap/cmd/cat.go b/go/cli/mcap/cmd/cat.go
index 1ea697cd93..08e9cfd253 100644
--- a/go/cli/mcap/cmd/cat.go
+++ b/go/cli/mcap/cmd/cat.go
@@ -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
 		}
diff --git a/go/cli/mcap/cmd/cat_test.go b/go/cli/mcap/cmd/cat_test.go
index 4e652810d0..8131bcf3bf 100644
--- a/go/cli/mcap/cmd/cat_test.go
+++ b/go/cli/mcap/cmd/cat_test.go
@@ -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)
diff --git a/go/cli/mcap/cmd/info.go b/go/cli/mcap/cmd/info.go
index d4688c94b3..234f30a4da 100644
--- a/go/cli/mcap/cmd/info.go
+++ b/go/cli/mcap/cmd/info.go
@@ -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 {
diff --git a/go/cli/mcap/cmd/info_test.go b/go/cli/mcap/cmd/info_test.go
new file mode 100644
index 0000000000..f42cf2a507
--- /dev/null
+++ b/go/cli/mcap/cmd/info_test.go
@@ -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)
+}