Fix out of bounds error for pprof default type

[dalehamel]

It is possible that the defaultSampleType index is out of bounds. This adds a bounds check to ensure it is a valid index.

[jlfwong]

Hi @dalehamel!

In what circumstances does this happen? It seems to me like this would constitute an invalid profile

I'm not opposed to the change, but want to understand why this happens to figure out e.g. if there's someplace upstream where this needs fixing

[dalehamel]

@jlfwong yeah i have only seen this happen when the pprof was built from scratch, often by a converter. I believe it happens if a ValueType is used in a sample but not added to the profile's value types, but it's been a while since i made the issue so I cannot remember the edge case that causes this.

Either way it is probably still a good idea to have the index validation, as it is still possible to view the profile.

[vasi-stripe]

We're actually using this field wrong!

It's documented as an index into the string_table, NOT into the repeated sample_types. So if you have a pprof containing both cpu & allocs, and the default type is allocs, it might look like:

string_table:
  0: ""
  1: "samples"
  2: "count"
  3: "alloc_objects"
sample_types:
  0:
    type: 1
    unit: 2
  1:
    type: 3
    unit: 2
default_sample_type: 3

[vasi-stripe]

Here's an example in google's pprof tool, where we're printing the debugging representation of a profile: https://github.com/google/pprof/blob/main/profile/profile.go#L566

Note how it's finding which type is the default by comparing type.Type == profile.DefaultSampleType—comparing the indexes into the string table, rather than looking at the index within the sample type array.

[vasi-stripe]

While trying to fix this, I'm running into a fun issue! When running in the browser, protobufjs seems to give me a number for DefaultSampleType—but when running in npm, it gives me a Long object.

This changes what this line does, making a DefaultSampleType appear truth-y in tests, and thus making the pprof/simple.prof choose sampleTypeIndex = 0 instead of sampleTypeIndex = length - 1 = 1.

I'm not familiar enough with JS compilers to figure out why the long module ends up included in the npm build, but not the build parcel creates.

[dalehamel]

thanks @vasi-stripe