Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/http/pprof: malformed delta profiles caused by CL 483137 #64566

Closed
prattmic opened this issue Dec 5, 2023 · 2 comments
Closed

net/http/pprof: malformed delta profiles caused by CL 483137 #64566

prattmic opened this issue Dec 5, 2023 · 2 comments
Assignees
@prattmic
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.22

Comments

@prattmic
Copy link
Member

prattmic commented Dec 5, 2023 •
edited

As of https://go.dev/cl/483137, internal/profile.(*Profile).postDecode returns early if the profile is "empty" (i.e., contains no samples).

The runtime can generate profiles with no samples. e.g., a mutex profile when there hasn't been any sampled contention.

Such profiles are still valid, despite containing no samples. e.g., they still include the sample types and mapping. As a result of this CL, the parsed Profile type of such a profile does not have any of its strings extracted from the string table. If such a profile it rewritten via Profile.Write, it will be missing string values.

e.g.,

Good (before):

{       # (perftools.profiles.Profile) size=1.1K
  sample_type: {        # (perftools.profiles.ValueType) size=4B
    type: 1
    unit: 2
  }     # sample_type[0]
  sample_type: {        # (perftools.profiles.ValueType) size=4B
    type: 3
    unit: 4
  }     # sample_type[1]
  mapping: {    # (perftools.profiles.Mapping) size=22B
    id      : 0x0000000000000001
    memory_start     : 0x00005615a44b0000       # [if nanoseconds]: 1 day 2 hours
    memory_limit     : 0x00005615a5087000       # [if nanoseconds]: 1 day 2 hours
    filename: 5
    build_id: 6
  }     # mapping[0]
  mapping: {    # (perftools.profiles.Mapping) size=22B
    id      : 0x0000000000000002
    memory_start     : 0x00007f2d65ae3000       # [if nanoseconds]: 1 day 14 hours
    memory_limit     : 0x00007f2d65ae6000       # [if nanoseconds]: 1 day 14 hours
    filename: 7
    build_id: 8
  }     # mapping[1]
... (more mappings)
  string_table       : [ "", "contentions", "count", "delay", "nanoseconds", ... mapping strings ... ]
  time_nanos : 1701802090774144022      # 1.48Ei; 0x179e03fc2a9c2416; [as nanoseconds]: 2023-12-05 13:48:10.774144022 -0500
  period_type: {        # (perftools.profiles.ValueType) size=4B
    type: 1
    unit: 2
  }     # period_type
  period     : 1
}

Bad (after):

{       # (perftools.profiles.Profile) size=338B
  sample_type: {        # (perftools.profiles.ValueType) size=0B
  }     # sample_type[0]
  sample_type: {        # (perftools.profiles.ValueType) size=0B
  }     # sample_type[1]
  mapping: {    # (perftools.profiles.Mapping) size=18B
    id      : 0x0000000000000001
    memory_start     : 0x0000564d94a0d000       # [if nanoseconds]: 1 day 2 hours
    memory_limit     : 0x0000564d955e4000       # [if nanoseconds]: 1 day 2 hours
  }     # mapping[0]
  mapping: {    # (perftools.profiles.Mapping) size=18B
    id      : 0x0000000000000002
    memory_start     : 0x00007ff6c92be000       # [if nanoseconds]: 1 day 15 hours
    memory_limit     : 0x00007ff6c92c1000       # [if nanoseconds]: 1 day 15 hours
  }     # mapping[1]
... (more mappings)
  string_table       : [ "" ]
  time_nanos : 1701800704276216955      # 1.48Ei; 0x179e02b958e4bc7b; [as nanoseconds]: 2023-12-05 13:25:04.276216955 -0500
  period     : 1
}

The only users of internal/profile.(*Profile).Parse are PGO and net/http/pprof's serveDeltaProfile.

PGO isn't very interesting with an empty profile, but empty profiles are still interesting when collecting delta profiles. pprof chokes when trying to parse one of these malformed profiles:

$ go tool pprof /tmp/p0929574757
problem fetching source profiles: profiles have empty common sample type list

I will send a fix for this.

cc @mvdan @cherrymui @golang/runtime
@prattmic prattmic added NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Dec 5, 2023
@prattmic prattmic added this to the Go1.22 milestone Dec 5, 2023
@prattmic prattmic self-assigned this Dec 5, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Dec 5, 2023
@Akinte2

This comment was marked as spam.

Sign in to view
@gopherbot
Copy link

Change https://go.dev/cl/547996 mentions this issue: internal/profile: fully decode proto even if there are no samples

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
@prattmic @ezz-no
internal/profile: fully decode proto even if there are no samples
9041385 
This is a partial revert of CL 483137.

CL 483137 started checking errors in postDecode, which is good. Now we
can catch more malformed pprof protos. However this made
TestEmptyProfile fail, so an early return was added when the profile was
"empty" (no samples).

Unfortunately, this was problematic. Profiles with no samples can still
be valid, but skipping postDecode meant that the resulting Profile was
missing values from the string table. In particular, net/http/pprof
needs to parse empty profiles in order to pass through the sample and
period types to a final output proto. CL 483137 broke this behavior.

internal/profile.Parse is only used in two places: in cmd/compile to
parse PGO pprof profiles, and in net/http/pprof to parse before/after
pprof profiles for delta profiles. In both cases, the input is never
literally empty (0 bytes). Even a pprof proto with no samples still
contains some header fields, such as sample and period type. Upstream
github.com/google/pprof/profile even has an explicit error on 0 byte
input, so `go tool pprof` will not support such an input.

Thus TestEmptyProfile was misleading; this profile doesn't need to
support empty input at all.

Resolve this by removing TestEmptyProfile and replacing it with an
explicit error on empty input, as upstream
github.com/google/pprof/profile has. For non-empty input, always run
postDecode to ensure the string table is processed.

TestConvertCPUProfileEmpty is reverted back to assert the values from
before CL 483137. Note that in this case "Empty" means no samples, not a
0 byte input.

Continue to allow empty files for PGO in order to minimize the chance of
last minute breakage if some users have empty files.

Fixes golang#64566.

Change-Id: I83a1f0200ae225ac6da0009d4b2431fe215b283f
Reviewed-on: https://go-review.googlesource.com/c/go/+/547996
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prattmic prattmic

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.22
Development

No branches or pull requests
3 participants
@prattmic @gopherbot @Akinte2