Skip to content

runtime: cgo callback on extra M treated as external code after nested cgo callback returns [1.23 backport] #72871

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
Closed
gopherbot opened this issue Mar 14, 2025 · 4 comments
Closed

runtime: cgo callback on extra M treated as external code after nested cgo callback returns [1.23 backport] #72871

gopherbot opened this issue Mar 14, 2025 · 4 comments
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Go1.23.8

Comments

@gopherbot
Copy link
Contributor

@prattmic requested issue #72870 to be considered for backport to the next 1.23 minor release.

@gopherbot Please backport to 1.23 and 1.24. This bug can cause arbitrary crashes when CPU profiling is enabled.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Mar 14, 2025
@gopherbot gopherbot mentioned this issue Mar 14, 2025
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 14, 2025
@gopherbot gopherbot added this to the Go1.23.8 milestone Mar 14, 2025
@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/658055 mentions this issue: [release-branch.go1.23] runtime: only set isExtraInC if there are no Go frames left

@prattmic
Copy link
Member

prattmic commented Mar 17, 2025 •
edited
Loading

A cherry-pick of https://go.dev/cl/658415 is also required to skip the new test on ports with broken profiling.

Edit: done, see below

@gopherbot
Copy link
Contributor Author

Change https://go.dev/cl/658435 mentions this issue: [release-branch.go1.23] runtime: skip TestCgoCallbackPprof on platforms with broken profiling

@cherrymui cherrymui added the CherryPickApproved Used during the release process for point releases label Mar 19, 2025
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Mar 19, 2025
@gopherbot
Copy link
Contributor Author

Closed by merging CL 658055 (commit c855149) to release-branch.go1.23.

gopherbot pushed a commit that referenced this issue Mar 25, 2025
@prattmic @gopherbot
[release-branch.go1.23] runtime: only set isExtraInC if there are no …
c855149 
…Go frames left

mp.isExtraInC is intended to indicate that this M has no Go frames at
all; it is entirely executing in C.

If there was a cgocallback to Go and then a cgocall to C, such that the
leaf frames are C, that is fine. e.g., traceback can handle this fine
with SetCgoTraceback (or by simply skipping the C frames).

However, we currently mismanage isExtraInC, unconditionally setting it
on return from cgocallback. This means that if there are two levels of
cgocallback, we end up running Go code with isExtraInC set.

1. C-created thread calls into Go function 1 (via cgocallback).
2. Go function 1 calls into C function 1 (via cgocall).
3. C function 1 calls into Go function 2 (via cgocallback).
4. Go function 2 returns back to C function 1 (returning via the remainder of cgocallback).
5. C function 1 returns back to Go function 1 (returning via the remainder of cgocall).
6. Go function 1 is now running with mp.isExtraInC == true.

The fix is simple; only set isExtraInC on return from cgocallback if
there are no more Go frames. There can't be more Go frames unless there
is an active cgocall out of the Go frames.

For #72870.
Fixes #72871.

Cq-Include-Trybots: luci.golang.try:go1.23-linux-amd64-longtest
Change-Id: I6a6a636c4e7ba75a29639d7036c5af3738033467
Reviewed-on: https://go-review.googlesource.com/c/go/+/658035
Reviewed-by: Cherry Mui <cherryyz@google.com>
Commit-Queue: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 577bb3d)
Reviewed-on: https://go-review.googlesource.com/c/go/+/658055
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
gopherbot pushed a commit that referenced this issue Mar 25, 2025
@prattmic @gopherbot
[release-branch.go1.23] runtime: skip TestCgoCallbackPprof on platfor…
2c0a0fc 
…ms with broken profiling

CL 658035 added TestCgoCallbackPprof, which is consistently failing on
solaris. runtime/pprof maintains a list of platforms where CPU profiling
does not work properly. Since this test requires CPU profiling, skip the
this test on those platforms.

For #72870.
For #72876.
For #72871.

Change-Id: I6a6a636cbf6b16abcbba8771178fe1d001be9d9b
Reviewed-on: https://go-review.googlesource.com/c/go/+/658415
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-on: https://go-review.googlesource.com/c/go/+/658435
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Bypass: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
None yet
Milestone
Go1.23.8
Development

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