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

cmd/compile: user symbols can be in reserved namespace #37762

Closed
Pr0Ger opened this issue Mar 9, 2020 · 37 comments
Closed

cmd/compile: user symbols can be in reserved namespace #37762

Pr0Ger opened this issue Mar 9, 2020 · 37 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Pr0Ger
Copy link

Pr0Ger commented Mar 9, 2020

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

$ go version
go version go1.14 darwin/amd64

What did you do?

package main

import "debug/gosym"

func main()  {
	s1 := gosym.Sym{Name: "go.uber.org/zap/zapcore.(*CheckedEntry).Write"}

	println(s1.PackageName())
}

What did you expect to see?

go.uber.org/zap/zapcore

What did you see instead?

empty string

I see problem is right here but since I'm not aware how linker uses these prefixes I can't figure out how to properly fix this condition

// A prefix of "type." and "go." is a compiler-generated symbol that doesn't belong to any package.
// See variable reservedimports in cmd/compile/internal/gc/subr.go
if strings.HasPrefix(name, "go.") || strings.HasPrefix(name, "type.") {
return ""
}

@ianlancetaylor
Copy link
Contributor

The debug/gosym package is intended to be used to read object files generated by the compiler. It's not intended to be used by creating your own instances of gosym.Sym.

Can you show us a complete program that demonstrates this problem when opening an object generated by the compiler? Please also include the sources for the object that you want to open. Thanks.

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Mar 9, 2020
@Pr0Ger
Copy link
Author

Pr0Ger commented Mar 9, 2020

Yeah, sure (go.uber.org/zap is a public available library):

package main

import (
	"debug/gosym"
	"debug/macho"
	"fmt"
	"os"
	"strings"

	"go.uber.org/zap"
)

func main() {
	logger, _ := zap.NewDevelopment()
	logger.Debug("using logger just a little bit")

	executable, _ := os.Executable()
	exe, _ := macho.Open(executable)

	pclndat, _ := exe.Section("__gopclntab").Data()
	symTabRaw, _ := exe.Section("__gosymtab").Data()

	pcln := gosym.NewLineTable(pclndat, exe.Section("__text").Addr)
	symTab, _ := gosym.NewTable(symTabRaw, pcln)
	for _, funk := range symTab.Funcs {
		if strings.Contains(funk.Name, "go.uber.org") {
			fmt.Printf("%s -> %s\n", funk.Name, funk.PackageName())
		}
	}
}

And output (just a few lines):

❯ go run test.go
go: finding module for package go.uber.org/zap
go: downloading go.uber.org/zap v1.14.0
go: found go.uber.org/zap in go.uber.org/zap v1.14.0
2020-03-09T22:16:31.280+0300	DEBUG	test_package_name/test.go:15	using logger just a little bit
go.uber.org/zap/buffer.(*Buffer).AppendString ->
go.uber.org/zap/buffer.(*Buffer).AppendBool ->
...

@ianlancetaylor
Copy link
Contributor

Thanks. If user packages can be in the compiler-reserved "go." namespace, that seems like a potential problem.

CC @griesemer @mdempsky @josharian @randall77

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Mar 9, 2020
@ianlancetaylor ianlancetaylor added this to the Go1.15 milestone Mar 9, 2020
@ianlancetaylor ianlancetaylor changed the title debug/gosym: PackageName() cmd/compile: user symbols can be in reserved namespace Mar 9, 2020
@mdempsky
Copy link
Member

mdempsky commented Mar 9, 2020

Bitten again by our ad hoc symbol mangling rules. :(

I think a short-term workaround could be debug/gosym only checks for "go." and "type." when it can't find a '/'. I don't think the compiler ever generates go.* or type.* symbols that contain a /.

This would still cause problems for top-level user package paths like "go.mystuff" (as opposed to "go.mystuff/dir"). Not sure how to best address that off hand.

Edit: Nevermind, I'm mistaken:

$ nm $(go tool -n compile) | grep 'go\..*/' | head -1
0000000000d9aca0 R go.itab.*cmd/compile/internal/gc.blockHeap,container/heap.Interface

@cuonglm
Copy link
Member

cuonglm commented Mar 11, 2020

Can we change go. to go.. instead for compiler generated symbols?

@cuonglm
Copy link
Member

cuonglm commented Mar 18, 2020

Can we change go. to go.. instead for compiler generated symbols?

Seems doable, I make a quick change to go.itab -> go..itab and it's compiled ok and test passed.

@odeke-em
Copy link
Member

Some 2 years ago, there was a somewhat similar issue #25113 reported and fixed by @aarzilli.
We had clashes with userland variables that collided with compiler generated names such as statictmp_0, and we had to rename from statictmp -> .stmp, from @josharian's suggestion in CL https://go-review.googlesource.com/c/go/+/142497. Seems like a similar mangling scheme could help here, as @cuonglm suggested in #37762 (comment), thank you!

@gopherbot
Copy link

Change https://golang.org/cl/226282 mentions this issue: cmd/compile,link: use "go.." for compiler genereated types

@gopherbot
Copy link

Change https://golang.org/cl/226283 mentions this issue: debug/gosym: correct checking condition for compiler generated symbols

@odeke-em
Copy link
Member

Howdy @jeremyfaller @randall77 @thanm @cuonglm? Kindly pinging you since @cuonglm's CLs were abandoned in lieu of the new linker landing, which made tests fail, however no confirmation on whether this issue has been addressed, and it is marked for Go1.15. Thank you.

@cuonglm
Copy link
Member

cuonglm commented May 26, 2020

@odeke-em I think it's better to leave it for go1.16 instead. This will make a large change in both compiler and linker.

@odeke-em
Copy link
Member

Awesome, thank you for the advice @cuonglm!

@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 26, 2020
@mdempsky
Copy link
Member

Rather than doubling the period, it would probably be more robust if we included a character that can't appear in user package paths at all. There's a whole bunch of reserved characters at https://golang.org/ref/spec#Import_declarations for us to choose.

For example, I think we can change go. and type. to go: and type:.

@cherrymui
Copy link
Member

One thing that affects the choice or the character is that the symbol names may go to the external linker or dynamic linker in some cases, and some linkers don't like certain characters in symbol names. We could let the Go linker rewrite them, but I'd rather not to.

@mdempsky
Copy link
Member

@cherrymui Do you happen to know/remember any examples of external linkers not liking certain characters?

I remembered Go switching from center dot and division to real dot and slash in linker symbols a long time back, and I thought I remembered it was related to linker (or maybe debugger) issues. But looking now, I can't find any evidence of that. It appears to have been motivated by consistency (a6736fa).

@cherrymui
Copy link
Member

Issue #19529 is one, where the external linker doesn't like the @ character.

@mdempsky
Copy link
Member

@cherrymui Interesting, thanks. As far as I can tell skimming https://www.akkadia.org/drepper/symbol-versioning and looking at strings -a /lib/x86_64-linux-gnu/libc.so.6, the @ isn't actually part of the ELF symbol. Instead it seems just like a UI convention (e.g., recognized by the assembler, and used by readelf for printing, but not nm -D). So it at least seems like @ should be safe. I'm curious what exactly the underlying issue there is.

If : was an issue though, I think we'd know about it because of struct tag conventions; for example:

$ nm `which go` | grep :
00000000007bfae0 T type..eq.struct { Logentry struct { Revision int64 "xml:\"revision,attr\""; Date string "xml:\"date\"" } "xml:\"logentry\"" }
00000000007bfa60 T type..eq.struct { Revision int64 "xml:\"revision,attr\""; Date string "xml:\"date\"" }

Looking at other reserved symbols, it seems like these get used for compiling the runtime, so presumably they're all safe: ()[]{},;*. So we could use one of those instead if we're concerned.

I wonder if it's worth putting special characters in a runtime struct tag or something to help smoke out linker issues.

@ianlancetaylor
Copy link
Contributor

You won't see an @ in a symbol name in a shared library or an executable, because they store the version information in .gnu.version sections, but you will see them in object files. That is how an object file indicates which version to use for a symbol. See https://www.airs.com/blog/archives/220 .

@mdempsky
Copy link
Member

Ah, I see. I thought the .gnu.version stuff was also used in object files. That makes sense then.

@ianlancetaylor
Copy link
Contributor

ianlancetaylor commented Sep 11, 2020

Thet probably should have used the section in object files too. I don't know why they didn't.

@aclements
Copy link
Member

Ping. Should we bump this to 1.17? Changing these prefixes to use a : sounds good to me, and is probably a reasonably easy fix, but doesn't seem worth the risk at this point in the freeze. Does someone want to queue up a wait-release CL for the change (maybe after the Russquake)?

@cuonglm
Copy link
Member

cuonglm commented Dec 7, 2020

Ping. Should we bump this to 1.17? Changing these prefixes to use a : sounds good to me, and is probably a reasonably easy fix, but doesn't seem worth the risk at this point in the freeze. Does someone want to queue up a wait-release CL for the change (maybe after the Russquake)?

I can make a CL, just need to decide which character to use. As @mdempsky's above comment, : can be problem because of struct tag

@aclements
Copy link
Member

Bumping to 1.17 because this seems like too much potential risk for relatively little reward. But feel free to send a CL and we can land it as soon as the tree opens.

@aclements aclements modified the milestones: Go1.16, Go1.17 Dec 15, 2020
@aclements aclements added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Dec 15, 2020
@ianlancetaylor
Copy link
Contributor

Looks like we didn't get to this for 1.17. Rolling forward another cycle.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.17, Go1.18 Apr 27, 2021
@ianlancetaylor ianlancetaylor added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Apr 27, 2021
@gopherbot
Copy link

Change https://golang.org/cl/317917 mentions this issue: all: use ":" for compiler generated symbols

@cuonglm
Copy link
Member

cuonglm commented May 7, 2021

@ianlancetaylor Sorry, I actually have the CL in my local, but forgot to send it early in go1.17 cycle.

I send it now for reviewing, and mark it for go1.18 (It's a big change, though, so sending it early for reviewing is better.).

@cuonglm
Copy link
Member

cuonglm commented May 8, 2021

@mdempsky @cherrymui trybot failed in CL 317917 due to this https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/pe-dll.c;h=e7b82ba6ffadf74dc1b9ee71dc13d48336941e51;hb=HEAD#l972

According to PE file format, the name format of forwarder RVA (symbols in other DLL) have "." in that, otherwise, the linker can't find it.

@cagedmantis
Copy link
Contributor

This issue is currently labeled as early-in-cycle for Go 1.18.
That time is now, so this is a friendly ping so the issue is looked at again.

@mknyszek
Copy link
Contributor

As per @cuonglm's comment on golang.org/cl/317917 (https://go-review.googlesource.com/c/go/+/317917/comments/d31ae680_570d8f68), punting to 1.19.

@mknyszek mknyszek modified the milestones: Go1.18, Go1.19 Nov 10, 2021
@gopherbot
Copy link

This issue is currently labeled as early-in-cycle for Go 1.19.
That time is now, so a friendly reminder to look at it again.

@thanm
Copy link
Contributor

thanm commented May 11, 2022

Based on the comments on the CL, punting this to 1.20.

@thanm thanm modified the milestones: Go1.19, Go1.20 May 11, 2022
@gopherbot
Copy link

Change https://go.dev/cl/422315 mentions this issue: debug/gosym: fix missing symbol version for 1.2+ pclntab

gopherbot pushed a commit that referenced this issue Aug 9, 2022
Updates #37762

Change-Id: Ib587f472304a04ebd9794666228f81ae6cb5c2a3
Reviewed-on: https://go-review.googlesource.com/c/go/+/422315
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
As it can't appear in user package paths.

There is a hack for handling "go:buildid" and "type:*" on windows/386.

Previously, windows/386 requires underscore prefix on external symbols,
but that's only applied for SHOSTOBJ/SUNDEFEXT or cgo export symbols.
"go.buildid" is STEXT, "type.*" is STYPE, thus they are not prefixed
with underscore.

In external linking mode, the external linker can't resolve them as
external symbols. But we are lucky that they have "." in their name,
so the external linker see them as Forwarder RVA exports. See:

 - https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#export-address-table
 - https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=ld/pe-dll.c;h=e7b82ba6ffadf74dc1b9ee71dc13d48336941e51;hb=HEAD#l972)

This CL changes "." to ":" in symbols name, so theses symbols can not be
found by external linker anymore. So a hacky way is adding the
underscore prefix for these 2 symbols. I don't have enough knowledge to
verify whether adding the underscore for all STEXT/STYPE symbols are
fine, even if it could be, that would be done in future CL.

Fixes golang#37762

Change-Id: I92eaaf24c0820926a36e0530fdb07b07af1fcc35
Reviewed-on: https://go-review.googlesource.com/c/go/+/317917
Reviewed-by: Than McIntosh <thanm@google.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
Updates golang#37762

Change-Id: Ib587f472304a04ebd9794666228f81ae6cb5c2a3
Reviewed-on: https://go-review.googlesource.com/c/go/+/422315
Auto-Submit: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
@golang golang locked and limited conversation to collaborators Aug 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests