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.

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

runtime/pprof: lostProfileEvent stack breaks gentraceback guarantee #38096

Closed
prattmic opened this issue Mar 26, 2020 · 4 comments
Closed

runtime/pprof: lostProfileEvent stack breaks gentraceback guarantee #38096

prattmic opened this issue Mar 26, 2020 · 4 comments
Assignees
@prattmic
Labels
FrozenDueToAge

Comments

@prattmic
Copy link
Member

gentraceback generates PCs which are usually following the CALL instruction. For those that aren't, it fixes up the PCs so that functions processing the output can unconditionally decrement the PC. See https://github.com/golang/go/blob/master/src/runtime/traceback.go#L343-L355.

When generating a fake "lost profile event" stack (https://github.com/golang/go/blob/master/src/runtime/pprof/proto.go#L325), addCPUData fails to meet this requirement, as lostProfileEvent-1 will land in the function preceeding lostProfileEvent.

#37447 exposes this bug as problematic. If the function preceeding lostProfileEvent contains inlined functions, then runtime_expandInlineFrames will panic when it tries to lookup the PC in the wrong function. Something like:

runtime: invalid pc-encoded table f=runtime/pprof.addMaxRSS pc=0x59145d targetpc=0x59145f tab=[0/0]0x0
        value=-1 until pc=0x59143c
        value=0 until pc=0x591443
        value=-1 until pc=0x59145d   
fatal error: invalid runtime symbol table

At HEAD, this isn't a problem because runtime/pprof.addMaxRSS precedes lostProfileEvent and contains no inlined calls, thus runtime_expandInlineFrames short-circuits. N.B., I modified addMaxRSS to add an inlined call for the panic above.

I'll need to double check if this is a problem for 1.14.1, but even if not, we probably want to patch 1.14.2, as subtle build order changes could break things.

cc @randall77 @heschik @hyangah
@prattmic prattmic self-assigned this Mar 26, 2020
@gopherbot
Copy link

Change https://golang.org/cl/225661 mentions this issue: runtime/pprof: increment fake overflow record PC

@prattmic
Copy link
Member Author

It looks like this could affect 1.14.1. lostProfileEvent is preceded by scaleMutexProfile, which inlines runtime.SetMaxProfileRate:

0000000000591680 <runtime/pprof.scaleMutexProfile>:
  591680:       48 8b 05 e1 0b 24 00    mov    0x240be1(%rip),%rax        # 7d2268 <runtime.mutexprofilerate>
  591687:       48 8b 4c 24 08          mov    0x8(%rsp),%rcx
  59168c:       48 0f af c8             imul   %rax,%rcx
  591690:       48 89 4c 24 18          mov    %rcx,0x18(%rsp)
  591695:       0f 57 c0                xorps  %xmm0,%xmm0
  591698:       f2 48 0f 2a c0          cvtsi2sd %rax,%xmm0
  59169d:       f2 0f 10 4c 24 10       movsd  0x10(%rsp),%xmm1
  5916a3:       f2 0f 59 c8             mulsd  %xmm0,%xmm1
  5916a7:       f2 0f 11 4c 24 20       movsd  %xmm1,0x20(%rsp)
  5916ad:       c3                      retq   
  5916ae:       cc                      int3   
  5916af:       cc                      int3   

00000000005916b0 <runtime/pprof.lostProfileEvent>:
  5916b0:       64 48 8b 0c 25 f8 ff    mov    %fs:0xfffffffffffffff8,%rcx
  5916b7:       ff ff 
  5916b9:       48 3b 61 10             cmp    0x10(%rcx),%rsp
  5916bd:       76 1a                   jbe    5916d9 <runtime/pprof.lostProfileEvent+0x29>
  5916bf:       48 83 ec 08             sub    $0x8,%rsp
  5916c3:       48 89 2c 24             mov    %rbp,(%rsp)
  5916c7:       48 8d 2c 24             lea    (%rsp),%rbp
  5916cb:       e8 e0 ff ff ff          callq  5916b0 <runtime/pprof.lostProfileEvent>
  5916d0:       48 8b 2c 24             mov    (%rsp),%rbp
  5916d4:       48 83 c4 08             add    $0x8,%rsp
  5916d8:       c3                      retq   
  5916d9:       e8 12 25 ed ff          callq  463bf0 <runtime.morestack_noctxt>
  5916de:       eb d0                   jmp    5916b0 <runtime/pprof.lostProfileEvent>

I'll send a cherry-pick request (though, per above, I think we want either way).

@prattmic
Copy link
Member Author

@gopherbot, please open a backport issue for 1.14.

@gopherbot gopherbot mentioned this issue Mar 27, 2020
Closed
@gopherbot
Copy link

Backport issue(s) opened: #38118 (for 1.14).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@golang golang locked and limited conversation to collaborators Mar 27, 2021
@prattmic prattmic self-assigned this Jun 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@prattmic prattmic

Labels
FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@prattmic @gopherbot