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: some linkname signatures do not match #58440

Closed
amscanne opened this issue Feb 9, 2023 · 6 comments
Closed

runtime: some linkname signatures do not match #58440

amscanne opened this issue Feb 9, 2023 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge

Comments

@amscanne
Copy link
Contributor

amscanne commented Feb 9, 2023

The two affected functions are:

  • internal/poll.runtime_pollWaitCanceled
  • sync/atomic.runtime_procPin

What version of Go are you using (go version)?

go1.19rc2

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

This was identified as an error by a linkname analyzer. A patch was posted, and a suggestion was made to file a specific bug.

What did you expect to see?

I expected signatures of these functions to match. If they did not, they should at least have the same shape (i.e. they may intentionally mismatch as in the case of runtime.ifaceI2E).

What did you see instead?

They were identical except return values (or lack thereof). This could cause problems with assumptions regarding the stack layout for those calls.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Feb 9, 2023
@prattmic
Copy link
Member

prattmic commented Feb 9, 2023

To add more detail:

internal/poll.runtime_pollWaitCanceled had an extra return value in the calling package. This means callers reserved an extra stack slot for the return value which the function would not set. This shouldn't cause problems unless the callers tried to use the non-existent return value (they don't). This bug was introduced in https://go.dev/cl/10485043 (the CL adding the symbol).

sync/atomic.runtime_procPin is missing a return value in the calling package. This means that callers do not reserve a stack slot for the return value, and the function is clobbering some other stack slot. This is scary, though as far as I know it has never caused problems [1]. This bug was introduced in https://go.dev/cl/136710045 (the CL adding the linkname).

[1] I suspect, though I haven't verified, that we are clobbering the stack slot for typ which is already dead by the call to procPin.

@prattmic
Copy link
Member

prattmic commented Feb 9, 2023

These are fixed in https://go.dev/cl/466615, which is already merged.

@prattmic prattmic closed this as completed Feb 9, 2023
@prattmic
Copy link
Member

prattmic commented Feb 9, 2023

@gopherbot please open a backport to 1.20 and 1.19. This bug has the opportunity to cause memory corruption in combination with other seemingly innocuous changes. We should backport for safety, as the fix is trivial.

This was referenced Feb 9, 2023
Closed
Closed
@gopherbot
Copy link

Backport issue(s) opened: #58441 (for 1.19), #58442 (for 1.20).

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

@amscanne
Copy link
Contributor Author

amscanne commented Feb 23, 2023 •
edited

A new instance of this bug was just introduced along with some metrics functionality. It actually changes the type of a function parameter (which is saved in the callee context), but presumably it is still compatible. I think it should be fixed either way.

I've posted a patch to fix this here: https://go-review.googlesource.com/c/go/+/470915

@gopherbot
Copy link

Change https://go.dev/cl/470915 mentions this issue: runtime: fix linkname signature for godebug

gopherbot pushed a commit that referenced this issue Feb 23, 2023
@amscanne @gopherbot
runtime: fix linkname signature for godebug
52fea4b 
This signature uses the wrong type for the passed function, which
will be saved in the internal runtime map. Since the functions are
likely compatible (uint64 return versus int64), this may work but
should generally be fixed.

This is other instance of #58440.

Change-Id: Ied82e554745ef72eefeb5be540605809ffa06533
Reviewed-on: https://go-review.googlesource.com/c/go/+/470915
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Michael Pratt <mpratt@google.com>
@golang golang locked and limited conversation to collaborators Feb 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Milestone
No milestone
Development

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