Skip to content

Commit

Permalink
Add bounds checking for sampleTypeIndex (jlfwong#449)
Browse files Browse the repository at this point in the history
Wow this was surprising. As reported in jlfwong#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 jlfwong#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 jlfwong#448
  • Loading branch information
jlfwong authored Dec 8, 2023
1 parent de17f12 commit ac4a015
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 1 deletion.
Original file line number Diff line number Diff line change
@@ -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}]}]}
12 changes: 11 additions & 1 deletion src/import/pprof.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -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)
}

Expand Down
54 changes: 54 additions & 0 deletions src/lib/__snapshots__/file-format.test.ts.snap
Original file line number Diff line number Diff line change
@@ -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 [
Expand Down
6 changes: 6 additions & 0 deletions src/lib/file-format.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
})
Expand Down

0 comments on commit ac4a015

Please sign in to comment.