gcplogs: forcibly set HOME on static UNIX binary #29478

AkihiroSuda · 2016-12-16T10:00:20Z

- 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)

AkihiroSuda · 2016-12-16T10:13:58Z

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 · 2016-12-16T10:19:14Z

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

AkihiroSuda · 2016-12-16T10:37:50Z

@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 · 2016-12-23T11:17:40Z

daemon/logger/gcplogs/gcplogging.go

This only really needs to be runtime.GOOS == "linux" as it is a glibc bug...

justincormack · 2016-12-23T11:18:29Z

pkg/idtools/idtools_windows.go

Do not need this comment on Windows

justincormack · 2016-12-23T11:34:22Z

Windows CI out of disk space.

justincormack · 2016-12-24T12:53:18Z

@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 · 2016-12-26T06:52:35Z

@thaJeztah @justincormack
Updated PR and now CI green

thaJeztah · 2016-12-27T15:26:17Z

pkg/homedir/homedir_linux.go

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

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(?) 😅 )

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 · 2016-12-28T16:55:45Z

Needs a rebase.

justincormack · 2016-12-28T18:22:06Z

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

Fix moby#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 <[email protected]>

AkihiroSuda · 2016-12-29T03:19:19Z

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

thaJeztah

LGTM, thanks!

stevvooe · 2017-12-01T02:36:44Z

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

AkihiroSuda · 2017-12-01T02:43:06Z

@stevvooe

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

stevvooe · 2017-12-01T02:44:28Z

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...

This commit also eliminates call for `os/user.Current()`, which segfaults when glibc is statically linkedin. (moby/moby#29478) Signed-off-by: Akihiro Suda <[email protected]>

GordonTheTurtle added the status/0-triage label Dec 16, 2016

AkihiroSuda mentioned this pull request Dec 16, 2016

Vendor gcplogs deps #29475

Merged

thaJeztah added status/2-code-review and removed status/0-triage labels Dec 16, 2016

AkihiroSuda mentioned this pull request Dec 16, 2016

gcplogs logging driver segfaults (static binary only) #29344

Closed

ehazlett added the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 19, 2016

justincormack reviewed Dec 23, 2016

View reviewed changes

daemon/logger/gcplogs/gcplogging.go Outdated

Copy link

Contributor

justincormack Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only really needs to be runtime.GOOS == "linux" as it is a glibc bug...

justincormack reviewed Dec 23, 2016

View reviewed changes

pkg/idtools/idtools_windows.go Outdated

Copy link

Contributor

justincormack Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not need this comment on Windows

AkihiroSuda force-pushed the fix-gcplogs-29344 branch from b9f3656 to 30c71d4 Compare December 26, 2016 03:14

AkihiroSuda removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 26, 2016

AkihiroSuda force-pushed the fix-gcplogs-29344 branch from 30c71d4 to 2bf2779 Compare December 26, 2016 03:17

justincormack approved these changes Dec 27, 2016

View reviewed changes

thaJeztah reviewed Dec 27, 2016

View reviewed changes

AkihiroSuda force-pushed the fix-gcplogs-29344 branch from 2bf2779 to b86e3be Compare December 29, 2016 03:17

thaJeztah added the impact/changelog label Dec 29, 2016

thaJeztah approved these changes Dec 29, 2016

View reviewed changes

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

GordonTheTurtle added this to the 1.14.0 milestone Dec 29, 2016

clnperez mentioned this pull request Jan 10, 2017

Add manifest command to docker cli #27455

Closed

6 tasks

yongtang mentioned this pull request Aug 30, 2017

[RFC]/[WIP] Update go to 1.9 #34671

Closed

yongtang mentioned this pull request Aug 30, 2017

Bump Go to 1.9.2 #33892

Merged

AkihiroSuda mentioned this pull request Dec 1, 2017

cmd/containerd-shim: require unix socket credentials containerd/containerd#1849

Merged

AkihiroSuda mentioned this pull request Dec 1, 2017

unixcreds: use euid instead of uid containerd/ttrpc#14

Merged

This was referenced Dec 31, 2022

daemon/logger/gcplogs: remove ensureHomeIfIAmStatic workaround #44722

Merged

[23.0 backport] daemon/logger/gcplogs: remove ensureHomeIfIAmStatic workaround #44724

Merged

gcplogs: forcibly set HOME on static UNIX binary #29478

gcplogs: forcibly set HOME on static UNIX binary #29478

Uh oh!

Conversation

AkihiroSuda commented Dec 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Dec 16, 2016

Uh oh!

thaJeztah commented Dec 16, 2016

Uh oh!

AkihiroSuda commented Dec 16, 2016

Uh oh!

justincormack Dec 23, 2016

Choose a reason for hiding this comment

Uh oh!

justincormack Dec 23, 2016

Choose a reason for hiding this comment

Uh oh!

justincormack commented Dec 23, 2016

Uh oh!

justincormack commented Dec 24, 2016

Uh oh!

AkihiroSuda commented Dec 26, 2016

Uh oh!

thaJeztah Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 27, 2016

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Dec 28, 2016

Uh oh!

justincormack commented Dec 28, 2016

Uh oh!

AkihiroSuda commented Dec 29, 2016

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Dec 1, 2017

Uh oh!

AkihiroSuda commented Dec 1, 2017

Uh oh!

stevvooe commented Dec 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AkihiroSuda commented Dec 16, 2016 •

edited

Loading