From ac4a0155596da13f3a76d752d63d06a31dc32ac0 Mon Sep 17 00:00:00 2001
From: Jamie Wong <jamie.lf.wong@gmail.com>
Date: Thu, 7 Dec 2023 18:43:47 -0800
Subject: [PATCH] Add bounds checking for sampleTypeIndex (#449)

Wow this was surprising. As reported in #448, the `simple.speedscope.json` file failed import. This was surprising to me because there's a test that covers that file to ensure it imports correctly.

The file provided in #448, however, is from a version of speedscope from 5 years ago before the file had been pretty printed. It turns out that when this *particular* file is not pretty-printed, it's a schematically valid pprof profile.

The fix is to do some bounds checks and return null. After the change, the file imports as you'd expect after realizing its not actually a valid pprof profile.

Fixes #448
---
 .../simple-no-pretty-print.speedscope.json    |  1 +
 src/import/pprof.ts                           | 12 ++++-
 .../__snapshots__/file-format.test.ts.snap    | 54 +++++++++++++++++++
 src/lib/file-format.test.ts                   |  6 +++
 4 files changed, 72 insertions(+), 1 deletion(-)
 create mode 100644 sample/profiles/speedscope/0.0.1/simple-no-pretty-print.speedscope.json

diff --git a/sample/profiles/speedscope/0.0.1/simple-no-pretty-print.speedscope.json b/sample/profiles/speedscope/0.0.1/simple-no-pretty-print.speedscope.json
new file mode 100644
index 0000000..2d98b82
--- /dev/null
+++ b/sample/profiles/speedscope/0.0.1/simple-no-pretty-print.speedscope.json
@@ -0,0 +1 @@
+{"version":"0.0.1","$schema":"https://www.speedscope.app/file-format-schema.json","shared":{"frames":[{"name":"a"},{"name":"b"},{"name":"c"},{"name":"d"}]},"profiles":[{"type":"evented","name":"simple.txt","unit":"none","startValue":0,"endValue":14,"events":[{"type":"O","frame":0,"at":0},{"type":"O","frame":1,"at":0},{"type":"O","frame":2,"at":0},{"type":"C","frame":2,"at":2},{"type":"O","frame":3,"at":2},{"type":"C","frame":3,"at":6},{"type":"O","frame":2,"at":6},{"type":"C","frame":2,"at":9},{"type":"C","frame":1,"at":14},{"type":"C","frame":0,"at":14}]}]}
\ No newline at end of file
diff --git a/src/import/pprof.ts b/src/import/pprof.ts
index d21f64f..c4c84e0 100644
--- a/src/import/pprof.ts
+++ b/src/import/pprof.ts
@@ -129,6 +129,11 @@ export function importAsPprofProfile(rawProfile: ArrayBuffer): Profile | null {
   }))
 
   const sampleTypeIndex = getSampleTypeIndex(protoProfile)
+
+  if (sampleTypeIndex < 0 || sampleTypeIndex >= sampleTypes.length) {
+    return null
+  }
+
   const sampleType = sampleTypes[sampleTypeIndex]
 
   const profileBuilder = new StackListProfileBuilder()
@@ -149,7 +154,12 @@ export function importAsPprofProfile(rawProfile: ArrayBuffer): Profile | null {
   for (let s of protoProfile.sample) {
     const stack = s.locationId ? s.locationId.map(l => frameByLocationID.get(i32(l))) : []
     stack.reverse()
-    const value = s.value![sampleTypeIndex]
+
+    if (s.value == null || s.value.length <= sampleTypeIndex) {
+      return null
+    }
+
+    const value = s.value[sampleTypeIndex]
     profileBuilder.appendSampleWithWeight(stack.filter(f => f != null) as FrameInfo[], +value)
   }
 
diff --git a/src/lib/__snapshots__/file-format.test.ts.snap b/src/lib/__snapshots__/file-format.test.ts.snap
index adf8b30..15a0199 100644
--- a/src/lib/__snapshots__/file-format.test.ts.snap
+++ b/src/lib/__snapshots__/file-format.test.ts.snap
@@ -1,5 +1,59 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`importSpeedscopeProfiles 0.0.1 evented profile (no pretty print) 1`] = `
+Object {
+  "frames": Array [
+    Frame {
+      "col": undefined,
+      "file": undefined,
+      "key": 0,
+      "line": undefined,
+      "name": "a",
+      "selfWeight": 0,
+      "totalWeight": 14,
+    },
+    Frame {
+      "col": undefined,
+      "file": undefined,
+      "key": 1,
+      "line": undefined,
+      "name": "b",
+      "selfWeight": 5,
+      "totalWeight": 14,
+    },
+    Frame {
+      "col": undefined,
+      "file": undefined,
+      "key": 2,
+      "line": undefined,
+      "name": "c",
+      "selfWeight": 5,
+      "totalWeight": 5,
+    },
+    Frame {
+      "col": undefined,
+      "file": undefined,
+      "key": 3,
+      "line": undefined,
+      "name": "d",
+      "selfWeight": 4,
+      "totalWeight": 4,
+    },
+  ],
+  "name": "simple.txt",
+  "stacks": Array [
+    "a;b;c 2",
+    "a;b;d 4",
+    "a;b;c 3",
+    "a;b 5",
+  ],
+}
+`;
+
+exports[`importSpeedscopeProfiles 0.0.1 evented profile (no pretty print): indexToView 1`] = `0`;
+
+exports[`importSpeedscopeProfiles 0.0.1 evented profile (no pretty print): profileGroup.name 1`] = `"simple.txt"`;
+
 exports[`importSpeedscopeProfiles 0.0.1 evented profile 1`] = `
 Object {
   "frames": Array [
diff --git a/src/lib/file-format.test.ts b/src/lib/file-format.test.ts
index d2765d6..cb15d4e 100644
--- a/src/lib/file-format.test.ts
+++ b/src/lib/file-format.test.ts
@@ -5,6 +5,12 @@ describe('importSpeedscopeProfiles', () => {
     await checkProfileSnapshot('./sample/profiles/speedscope/0.0.1/simple.speedscope.json')
   })
 
+  test('0.0.1 evented profile (no pretty print)', async () => {
+    await checkProfileSnapshot(
+      './sample/profiles/speedscope/0.0.1/simple-no-pretty-print.speedscope.json',
+    )
+  })
+
   test('0.1.2 sampled profile', async () => {
     await checkProfileSnapshot('./sample/profiles/speedscope/0.1.2/simple-sampled.speedscope.json')
   })