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/link: detect -X setting non-string variable? #9621

Closed
jessfraz opened this issue Jan 17, 2015 · 23 comments

Comments

Projects
None yet
8 participants
@jessfraz
Copy link
Contributor

commented Jan 17, 2015

Ok this is a bit of an oddball bug. We tried for at least 3 hours to find a small reproducible case to give you all but could not find a way to do it.

We just switched the Docker project, https://github.com/docker/docker, to use go 1.4.1. The problem we are seeing is a few patches that have been submitted are compiling successfully but the resulting binary is failing to work. It cannot find the linked files, etc. But I should note, the master branch of docker is working correctly with Go1.4.1.

Some background on how we compile docker:

We have found that we can get Docker to compile correctly on these failed patches, by adding an empty

func init(){
}

to the bottom of the dockerversion.go file which is the file being compiled with -X dockerversion.IAMSTATIC in the above script linked to our build process.

So we have an idea:

  • Maybe there is some compiler optimization we are hitting. The thoughts behind this stem from the fact, the variables in the dockerversion.go file are empty and get filled at the linker stage. But this theory is quite odd because as mentioned before, the master branch works with go1.4.1.

The patches that fail are:

These are not abnormal patches in anyway, we can't find any commonality between them that would cause this.

To sum up, all patches work successfully on Go1.4.0, but fail on 1.4.1. Also by fail I mean they compile with no errors but when trying to run the resulting binary it does not work because it cannot find the linked files.

cc @tianon @icecrime

Also again, apologies we could not find a simple reproducible case. Let me know anything else I can do information wise. and I welcome those I pinged above to add anything I may have forgotten or explained poorly.

Reproducing

Making a binary

$ git clone git@github.com:docker/docker
$ cd docker
$ ./hack/make.sh binary

# running the binary, this should fail with the patches
$ ./bundles/1.4.1-dev/binary/docker -d -D

Running the integration suite

These are the tests that run on the compiled binary. All should fail when you apply any of the above patches.

$ ./hack/make.sh test-integration-cli 
@minux

This comment has been minimized.

Copy link
Member

commented Jan 17, 2015

The docker project is so huge that I can't easily reproduce the problem.

As there isn't many patches backported to Go 1.4.1, could you please
try to bisect exactly which commit breaks your build? Thanks.

Going through the history, I didn't see any suspicious commit that could
affect the -X flag, unless you're using some hacks with syso files, as
the only cmd/ld patch affects linking .syso files.

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2015

TLDR:

Compiling a static binary with go 1.4 works, and the binary works
Compiling a static binary with go 1.4.1 works, running it fails with not being able to find linked files

However the second statement is a craps shoot. Sometimes it works and sometimes it doesn't. Which is described in detail above.

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2015

Yes I will do a bisect. Sorry.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2015

On 17 Jan 2015, at 14:01, Jessie Frazelle notifications@github.com wrote:

TLDR:

Compiling a static binary with go 1.4 works, and the binary works
Compiling a static binary with go 1.4.1 works, running it fails with not being able to find linked files

That suggests to me that you don't have a static binary.

Can you post the output of the failure. I'll probably get shouted down for suggesting this, but also the output of ldd $binary

However the second statement is a craps shoot. Sometimes it works and sometimes it doesn't. Which is described in detail above.


Reply to this email directly or view it on GitHub.

@mikioh mikioh changed the title Compiling with Go1.4.1 having binary problems build: compiling with go1.4.1 having binary problems Jan 17, 2015

@tianon

This comment has been minimized.

Copy link

commented Jan 17, 2015

Hopefully some of this helps, @davecheney: (while @jfrazelle works on that bisect)

$ # in docker/docker, https://github.com/docker/docker/pull/10151 checked out
$ git log -1 --oneline
fa2024b Add ability to set client runtime defaults
$ make shell
docker build ...
...
docker run ...
root@c2af08758a22:/go/src/github.com/docker/docker# git grep -n IAMSTATIC
dockerversion/dockerversion.go:12:  IAMSTATIC bool   // whether or not Docker itself was compiled statically via ./hack/make.sh binary
project/make.sh:127:    -X $DOCKER_PKG/dockerversion.IAMSTATIC true
utils/utils.go:97:  if dockerversion.IAMSTATIC {
root@c2af08758a22:/go/src/github.com/docker/docker# bash -x ./hack/make.sh binary
...
++ go build -o /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev -a -tags 'netgo static_build apparmor selinux btrfs_noversion daemon' -ldflags '
        -w 
    -X github.com/docker/docker/dockerversion.GITCOMMIT "fa2024b"
    -X github.com/docker/docker/dockerversion.VERSION "1.4.1-dev"


    -linkmode external
    -X github.com/docker/docker/dockerversion.IAMSTATIC true
    -extldflags "-static -lpthread -Wl,--unresolved-symbols=ignore-in-object-files"

    ' ./docker
++ echo 'Created binary: /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev'
...
root@c2af08758a22:/go/src/github.com/docker/docker# /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev -v
Docker version 1.4.1-dev, build fa2024b
root@c2af08758a22:/go/src/github.com/docker/docker# /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev -d
INFO[0000] +job serveapi(unix:///var/run/docker.sock)   
INFO[0000] Listening for HTTP on unix (/var/run/docker.sock) 
INFO[0000] +job init_networkdriver()                    
INFO[0000] enableIPv6 = false                           
INFO[0000] -job init_networkdriver() = OK (0)           
FATA[0000] Could not locate dockerinit: This usually means docker was built incorrectly. See http://docs.docker.com/contributing/devenvironment for official build instructions. 
root@c2af08758a22:/go/src/github.com/docker/docker# ldd /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev
    not a dynamic executable

So somehow, it's not getting the value of dockerversion.IAMSTATIC which is set to true via -ldflags -X (but it is getting the values of dockerversion.GITCOMMIT and dockerversion.VERSION, as seen from the -v output above).

If we do any of a number of small things, the problem goes away and the daemon starts just fine, including things as simple as:

  1. adding dockerversion.IAMSTATIC to the docker -v output in docker/docker.go or to the dockerinit error message output in daemon/daemon.go
  2. adding func init() { } (yes, empty) in dockerversion/dockerversion.go
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2015

@tianon is it possible for you to show two binaries, the working and the failed ?

@tianon

This comment has been minimized.

Copy link

commented Jan 17, 2015

Absolutely!

root@8280b2f578af:/go/src/github.com/docker/docker# git diff
diff --git a/dockerversion/dockerversion.go b/dockerversion/dockerversion.go
index c130ac2..687382f 100644
--- a/dockerversion/dockerversion.go
+++ b/dockerversion/dockerversion.go
@@ -1,5 +1,8 @@
 package dockerversion

+func init() {
+}
+
 // FIXME: this should be embedded in the docker/docker.go,
 // but we can't because distro policy requires us to
 // package a separate dockerinit binary, and that binary needs
root@8280b2f578af:/go/src/github.com/docker/docker# ./hack/make.sh binary

bundles/1.4.1-dev already exists. Removing.

---> Making bundle: binary (in bundles/1.4.1-dev/binary)
Created binary: /go/src/github.com/docker/docker/bundles/1.4.1-dev/binary/docker-1.4.1-dev

root@8280b2f578af:/go/src/github.com/docker/docker# ./bundles/1.4.1-dev/binary/docker -d
INFO[0000] +job serveapi(unix:///var/run/docker.sock)   
INFO[0000] Listening for HTTP on unix (/var/run/docker.sock) 
INFO[0000] +job init_networkdriver()                    
INFO[0000] enableIPv6 = false                           
INFO[0000] -job init_networkdriver() = OK (0)           
INFO[0000] Loading containers: start.                   

INFO[0000] Loading containers: done.                    
INFO[0000] docker daemon: 1.4.1-dev fa2024b-dirty; execdriver: native-0.2; graphdriver: btrfs 
INFO[0000] +job acceptconnections()                     
INFO[0000] -job acceptconnections() = OK (0)            
^CINFO[0013] Received signal 'interrupt', starting shutdown of docker... 
root@8280b2f578af:/go/src/github.com/docker/docker# # used Ctrl+C, since it hangs waiting for connections on the socket, running successfully as a daemon since it found the IAMSTATIC flag set to true

I can get you copies of the binaries too, if that'd be helpful.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2015

@tianon i'm sorry but I don't know much about the docket build process so quoting these large outputs is not informative.

Could you please show a minimum output that demonstrates the problem. My best guess so far is you built a go binary that was not static when it was expected to be. Is this correct ?

@tianon

This comment has been minimized.

Copy link

commented Jan 17, 2015

No, the problem is that one of our -X-injected variables isn't getting set -- we're getting static binaries just fine (both the working and the non-working binary ends up static).

We inject three variables there: dockerversion.VERSION, dockerversion.GITCOMMIT, and dockerversion.IAMSTATIC.

The first two are set to strings, which as you can see from the docker -v output come through just fine in either case. The one that doesn't is dockerversion.IAMSTATIC which is a bool that we set to true just to inform our daemon that it's a single-binary static build instead of a dual-binary dynamic build (with a pure-static smaller dockerinit binary, but that's not relevant here, it's just the context for what these variables are for -- the canonical way we build for our official releases is the single pure-static binary, which is where we're seeing the issue).

@tianon

This comment has been minimized.

Copy link

commented Jan 17, 2015

We've tried creating a smaller reproducing case than Docker itself, but we haven't had any success, especially since we haven't a clue what's causing it due to the strange things that cause it to be fixed (like adding an empty init() function in that otherwise just-vars package, which are all empty vars that get injected in the linker via -X).

@minux

This comment has been minimized.

Copy link
Member

commented Jan 17, 2015

Wait, I just found that your IAMSTATIC variable is declared bool.

I understand what's happening now.

cmd/ld -X only supports setting string variables, so what really
happens is that the linker changes the type to string and layout
the following in the binary for variable IAMSTATIC:

IAMSTATIC:
*byte // pointer to the string content
int // length of the string

But any Go code referencing the IAMSTATIC variable will still
think it's a bool, just load the first byte it and see if it's non-zero.
On x86, the first byte happens to be the least significant byte of
the string address.

If it happens that the "true" string is laid out at some address
whose least-significant byte is 0 (i.e. aligned to 256-byte), the
binary will read a false IAMSTATIC.

Please change the IAMSTATIC variable to a string, and
use a string == "true". This is not a Go bug. I will see if cmd/ld
could detect the misuse, but no promises.

@tianon

This comment has been minimized.

Copy link

commented Jan 17, 2015

@minux oh man, that's genius -- didn't even think about that; testing now

@minux

This comment has been minimized.

Copy link
Member

commented Jan 17, 2015

You can verify my explanation easily, assume docker is a failing
binary.

$ gdb ./docker
(gdb) x/16bx &'github.com/docker/docker/dockerversion.IAMSTATIC'

The first byte shown should be zero. And you will see that the first 8 byte
is an address, and the next 8 byte represents 4 (the length of "true").

@minux minux changed the title build: compiling with go1.4.1 having binary problems cmd/ld: detect -X setting non-string variable? Jan 17, 2015

@minux minux added the repo-main label Jan 17, 2015

@minux minux added this to the Go1.5 milestone Jan 17, 2015

@tianon

This comment has been minimized.

Copy link

commented Jan 17, 2015

Ahh, that does it. ❤️

Once IAMSTATIC is a string instead, all is well and right with the world. Didn't even realize we were using -X wrong. 😊

Indeed:

root@8280b2f578af:/go/src/github.com/docker/docker# gdb ./bundles/1.4.1-dev/binary/docker-1.4.1-dev
...
(gdb) x/16bx &'github.com/docker/docker/dockerversion.IAMSTATIC'
0x128c920 <github.com/docker/docker/dockerversion.IAMSTATIC>:   0x00    0x9a    0xe3    0x00    0x00    0x00    0x00    0x00
0x128c928 <github.com/docker/docker/dockerversion.IAMSTATIC+8>: 0x04    0x00    0x00    0x00    0x00    0x00    0x00    0x00
@jessfraz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2015

I still don't understand how it was ever working then haha but thanks so much, I will close this.

@jessfraz jessfraz closed this Jan 17, 2015

@jessfraz jessfraz reopened this Jan 17, 2015

@jessfraz

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2015

Sorry didn't realize you changed the title to keep open for other reasons.

@minux

This comment has been minimized.

Copy link
Member

commented Jan 17, 2015

For the cmd/ld issue, the only thing we can do is to detect if:

  1. the size of the object being set is the size of a string
  2. the object being set is in the DATA section, rather than NOPTRDATA
    or RODATA.

Either of these check will catch the OP's problem, but they still can't
catch every possible misuse.

I will leave this to other to decide.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2015

@minux they won't catch every possible misuse, but there aren't many NOPTRDATA/RODATA string-sized objects around. If the check is cheap and small, it seems worth doing.

@minux

This comment has been minimized.

Copy link
Member

commented Feb 5, 2015

@titanous

This comment has been minimized.

Copy link
Member

commented Feb 5, 2015

Please don't require the variable to be defined, we have build system sets a version variable on a specific package for all binaries built, and only some of them import the package to get access to the version.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2015

Requiring the variable to be defined was discussed and rejected in #8820.

We could however warn in the case in which there is no package name prefix at all.

@rsc rsc removed the repo-main label Apr 14, 2015

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jun 8, 2015

It should be easy to accept both undefined variables and variables declared as strings but nothing else.

@rsc rsc changed the title cmd/ld: detect -X setting non-string variable? cmd/link: detect -X setting non-string variable? Jun 8, 2015

@gopherbot

This comment has been minimized.

Copy link

commented Jun 29, 2015

CL https://golang.org/cl/11694 mentions this issue.

@rsc rsc closed this in 69f0d4c Jun 29, 2015

@golang golang locked and limited conversation to collaborators Jun 28, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.