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

gcplogs: forcibly set HOME on static UNIX binary #29478

Merged
merged 1 commit into from Dec 29, 2016

Conversation

Projects
None yet
7 participants
@AkihiroSuda
Member

AkihiroSuda commented Dec 16, 2016

- What I did
Fix #29344

The PR consists of 3 commits, but the first and the second one are dupe of #29475 for vendoring golang/oauth2@96382aa .
Please review the latest commit in this PR. (I will rebase this PR when #29475 is merged)

EDIT: rebased

- How I did it

If HOME is not set, the gcplogs logging driver will call os/user.Current() via oauth2/google.
However, in static binary, os/user.Current() leads to segfault due to a glibc issue that won't be fixed
in a short term. (golang/go#13470, https://sourceware.org/bugzilla/show_bug.cgi?id=19341)

So this PR forcibly sets HOME so as to avoid call to os/user/Current().

- How to verify it

  • Build the static daemon binary (make binary)
  • Start the daemon without HOME (typically, just starting the daemon via systemd is enough)
  • Make sure the daemon prints a warning log: level=warning msg="gcplogs requires HOME to be set for static daemon binary. Forcibly setting HOME to /root."
  • Do docker run --rm --log-driver=gcplogs busybox echo hello many times and make sure all of them succeeds without SEGV

- Description for the changelog

gcplogs: forcibly set HOME on static UNIX binary

- A picture of a cute animal (not mandatory but encouraged)

image

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 16, 2016

Member

windowsRS1 failing because go-autogen.ps1 does not add IAmStatic

00:08:18.526 # github.com/docker/docker/daemon/logger/gcplogs
00:08:18.527 daemon\logger\gcplogs\gcplogging.go:126: undefined: dockerversion.IAmStatic

I'll fix later

Member

AkihiroSuda commented Dec 16, 2016

windowsRS1 failing because go-autogen.ps1 does not add IAmStatic

00:08:18.526 # github.com/docker/docker/daemon/logger/gcplogs
00:08:18.527 daemon\logger\gcplogs\gcplogging.go:126: undefined: dockerversion.IAmStatic

I'll fix later

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 16, 2016

Member

Is this something to integrate into https://github.com/docker/docker/blob/master/pkg/homedir/homedir.go, or better kept separate?

Member

thaJeztah commented Dec 16, 2016

Is this something to integrate into https://github.com/docker/docker/blob/master/pkg/homedir/homedir.go, or better kept separate?

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 16, 2016

Member

@thaJeztah
Ah, I didn't notice the homedir pkg.
I'm ok to move the function to the homedir pkg, but it will be a separate function.

Existing homedir.Get():

  • calls os/user.Current()
  • Should work with non-file database (e.g. LDAP)

The function I'll add to homedir (I could not come up with a good function name.. Maybe GetStatic?)

  • directly read /etc/passwd
  • Should not work with LDAP
Member

AkihiroSuda commented Dec 16, 2016

@thaJeztah
Ah, I didn't notice the homedir pkg.
I'm ok to move the function to the homedir pkg, but it will be a separate function.

Existing homedir.Get():

  • calls os/user.Current()
  • Should work with non-file database (e.g. LDAP)

The function I'll add to homedir (I could not come up with a good function name.. Maybe GetStatic?)

  • directly read /etc/passwd
  • Should not work with LDAP
@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Dec 23, 2016

Contributor

Windows CI out of disk space.

Contributor

justincormack commented Dec 23, 2016

Windows CI out of disk space.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Dec 24, 2016

Contributor

@AkihiroSuda ok windows running again but the test failure is real:

12:17:01 # github.com/docker/docker/daemon/logger/gcplogs
12:17:01 daemon\logger\gcplogs\gcplogging.go:126: undefined: dockerversion.IAmStatic
Contributor

justincormack commented Dec 24, 2016

@AkihiroSuda ok windows running again but the test failure is real:

12:17:01 # github.com/docker/docker/daemon/logger/gcplogs
12:17:01 daemon\logger\gcplogs\gcplogging.go:126: undefined: dockerversion.IAmStatic
@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 26, 2016

Member

@thaJeztah @justincormack
Updated PR and now CI green

Member

AkihiroSuda commented Dec 26, 2016

@thaJeztah @justincormack
Updated PR and now CI green

// system, because a call to os/user.Current() in a static binary leads to
// segfault due to a glibc issue that won't be fixed in a short term.
// (#29344, golang/go#13470, https://sourceware.org/bugzilla/show_bug.cgi?id=19341)
func GetStatic() (string, error) {

This comment has been minimized.

@thaJeztah

thaJeztah Dec 27, 2016

Member

Wondering; should we have a dummy GetStatic() in non-linux files? What's the general approach in this situation?

@thaJeztah

thaJeztah Dec 27, 2016

Member

Wondering; should we have a dummy GetStatic() in non-linux files? What's the general approach in this situation?

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Dec 27, 2016

Member

Since the issue is specific to glibc's TLS handling, I think we do not need dummies for non-linux files, but I can add them if needed.

(I'm not sure this is an issue for non-Linux glibc-based systems. e.g. Debian GNU/kFreeBSD(?) 😅 )

@AkihiroSuda

AkihiroSuda Dec 27, 2016

Member

Since the issue is specific to glibc's TLS handling, I think we do not need dummies for non-linux files, but I can add them if needed.

(I'm not sure this is an issue for non-Linux glibc-based systems. e.g. Debian GNU/kFreeBSD(?) 😅 )

This comment has been minimized.

@thaJeztah

thaJeztah Dec 27, 2016

Member

It was more that GetStatic() is an exported function, so I was wondering if it's common practice to have an exported function only available on some platforms/architectures.

Change itself looks good to me

@thaJeztah

thaJeztah Dec 27, 2016

Member

It was more that GetStatic() is an exported function, so I was wondering if it's common practice to have an exported function only available on some platforms/architectures.

Change itself looks good to me

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Dec 28, 2016

Contributor

Needs a rebase.

Contributor

cpuguy83 commented Dec 28, 2016

Needs a rebase.

@justincormack

This comment has been minimized.

Show comment
Hide comment
@justincormack

justincormack Dec 28, 2016

Contributor

It needs the first two commits removing as #29475 is merged

Contributor

justincormack commented Dec 28, 2016

It needs the first two commits removing as #29475 is merged

gcplogs: forcibly set HOME on static UNIX binary
Fix #29344

If HOME is not set, the gcplogs logging driver will call os/user.Current() via oauth2/google.
However, in static binary, os/user.Current() leads to segfault due to a glibc issue that won't be fixed
in a short term. (golang/go#13470, https://sourceware.org/bugzilla/show_bug.cgi?id=19341)
So we forcibly set HOME so as to avoid call to os/user/Current().

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 29, 2016

Member

@thaJeztah @cpuguy83 @justincormack Rebased and added dummy pkg/homedir/homedir_others.go

Member

AkihiroSuda commented Dec 29, 2016

@thaJeztah @cpuguy83 @justincormack Rebased and added dummy pkg/homedir/homedir_others.go

@thaJeztah

LGTM, thanks!

@thaJeztah thaJeztah merged commit f635cf0 into moby:master Dec 29, 2016

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 28954 has succeeded
Details
janky Jenkins build Docker-PRs 37548 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 8564 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Dec 29, 2016

@clnperez clnperez referenced this pull request Jan 10, 2017

Closed

Add manifest command to docker cli #27455

3 of 6 tasks complete

This was referenced Aug 30, 2017

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Dec 1, 2017

Contributor

At this point, the bug doesn't seem to exist anymore. What are the plans to back out this code?

Contributor

stevvooe commented Dec 1, 2017

At this point, the bug doesn't seem to exist anymore. What are the plans to back out this code?

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 1, 2017

Member

@stevvooe

How was this fixed?
The status of https://sourceware.org/bugzilla/show_bug.cgi?id=19341 is still SUSPENDED.

Member

AkihiroSuda commented Dec 1, 2017

@stevvooe

How was this fixed?
The status of https://sourceware.org/bugzilla/show_bug.cgi?id=19341 is still SUSPENDED.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Dec 1, 2017

Contributor

Impossible to tell. I guess not, as I was able to reproduce.

Why do we continue to use broken glibc? What a constant piece of junk...

Contributor

stevvooe commented Dec 1, 2017

Impossible to tell. I guess not, as I was able to reproduce.

Why do we continue to use broken glibc? What a constant piece of junk...

AkihiroSuda added a commit to AkihiroSuda/ttrpc that referenced this pull request Dec 1, 2017

unixcreds: use euid instead of uid
This commit also eliminates call for `os/user.Current()`,
which segfaults when glibc is statically linkedin.
(moby/moby#29478)

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment