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

runtime: deeply nested struct initialized with non-zero values #46653

Closed
d-reinhold opened this issue Jun 8, 2021 · 10 comments
Closed

runtime: deeply nested struct initialized with non-zero values #46653

d-reinhold opened this issue Jun 8, 2021 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@d-reinhold
Copy link

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

$ go version
go version go1.16.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes, the issue is reproducible on the go playground (1.16.5).

What did you do?

This program instantiates a deeply nested struct by accessing an empty map.

What did you expect to see?

Per the go spec, this access should return a zero valued instance of the struct type.

What did you see instead?

The string fields five levels deep in the struct are initialized with values of enormous lengths:

Screen Shot 2021-06-08 at 12 26 02 PM
The program does not crash.

A slightly different version of this program forces the runtime stack to expand after initializing the corrupt struct, which causes the program to crash (this "bad pointer in frame" crash occurred in a production application, which motivated this investigation):

Screen Shot 2021-06-08 at 12 30 38 PM

Slightly changing the depth/breadth of the nested struct can cause the issue to disappear, or can cause other, stranger behaviors:

In the context of my application, I have also seen the string fields be initialized with real string values from somewhere else in the application's memory:

Screen Shot 2021-06-08 at 12 33 08 PM

At other times, the application has consumed all system memory attempting to print the struct:
Screen Shot 2021-06-08 at 1 07 11 PM

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 8, 2021
@seankhliao
Copy link
Member

@thanm
Copy link
Contributor

thanm commented Jun 8, 2021

This looks like a linker / objwriter issue.

Building crash.go with "-S", I see:

...
go.map.zero SRODATA dupok size=4096
...

however when I inspect the binary, the map.zero symbol has a smaller size:

objdump -t main | fgrep map.zero
00000000004dfd00 g     O .rodata	0000000000000420 go.map.zero

I think what's happening here is that because the symbol is marked as a regular DUPOK symbol as opposed to having hashed content, there are multiple go.map.zero symbols at link time, and we wind up picking a smaller one.

There is actually code in the linker to detect this problem (under debug option -strictdups) but the code written to check data payload length and not symbol size (in this case there is no payload since it's essentially a BSS symbol). Bummer.

I'll work on a fix.

@cherrymui cherrymui added this to the Go1.17 milestone Jun 8, 2021
@gopherbot
Copy link

Change https://golang.org/cl/326211 mentions this issue: cmd/compile: make map.zero symbol content-addressable

@gopherbot
Copy link

Change https://golang.org/cl/326210 mentions this issue: cmd/link: fix bug in -strictdups checking of BSS symbols

@thanm
Copy link
Contributor

thanm commented Jun 9, 2021

@gopherbot please consider this for backport to 1.15 and 1.16

@gopherbot
Copy link

Backport issue(s) opened: #46656 (for 1.15), #46657 (for 1.16).

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

@thanm thanm self-assigned this Jun 9, 2021
@thanm thanm 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 Jun 9, 2021
@thanm
Copy link
Contributor

thanm commented Jun 9, 2021

@d-reinhold thank you for reporting this bug. I have a tentative fix. It looks like this bug is also present in 1.15 and 1.16, so I will consult with the release team regarding back-porting the fix.

gopherbot pushed a commit that referenced this issue Jun 9, 2021
The linker's -strictdups debugging option was not properly checking
for cases where you have two dupok BSS symbols with different length
(the check examined data length and content, but not symbol size).

Updates #46653.

Change-Id: I3844f25ef76dd6e4a84ffd5caed5d19a1b1a57c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/326210
Trust: Than McIntosh <thanm@google.com>
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@d-reinhold
Copy link
Author

@thanm Thanks for the quick fix! Looks like it's working against my test-case on master. Those backports will be appreciated as well!

@gopherbot
Copy link

Change https://golang.org/cl/326212 mentions this issue: cmd/compile: make map.zero symbol content-addressable

@gopherbot
Copy link

Change https://golang.org/cl/326213 mentions this issue: cmd/compile: make map.zero symbol content-addressable

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants