Skip to content

Revert "homedir: add cgo or osusergo buildtag constraints for unix"#40134

Merged
tiborvass merged 2 commits intomoby:masterfrom
kolyshkin:homedir-osusergo
Oct 25, 2019
Merged

Revert "homedir: add cgo or osusergo buildtag constraints for unix"#40134
tiborvass merged 2 commits intomoby:masterfrom
kolyshkin:homedir-osusergo

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 24, 2019 •
edited
Loading

This reverts #39994 and slightly clarifies doc update of homedir.Get() added by #39975

Revert "homedir: add cgo or osusergo buildtag constraints for unix"

TL;DR: there is no way to do this right.

We do know that in some combination of build tags set (or unset),
linker flags, environment variables, and libc implementation, this package
won't work right. In fact, there is one specific combination:

  1. CGO_ENABLED=1 (or unset)
  2. static binary is being built (e.g. go build is run with -extldflags -static)
  3. go build links the binary against glibc
  4. osusergo is not set

This particular combination results in the following legitimate linker warning:

cgo_lookup_unix.go: warning: Using 'getpwuid_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

If this warning is ignored and the resulting binary is used on a system
with files from a different glibc version (or without those files), it
could result in a segfault.

The commit being reverted tried to guard against such possibility,
but the problem is, we can only use build tags to account for items
1 and 4 from the above list, while items 2 and 3 do not result in
any build tags being set or unset, making this guard excessive.

Remove it.

This reverts commit 023b072.

pkg/homedir: clarify Get() docs wrt static linking

This clarifies comments about static linking made in commit a8608b5.

  • There are two ways to create a static binary, one is to disable
    cgo, the other is to set linker flags. When cgo is disabled,
    there is no need to use osusergo build tag.

  • osusergo only needs to be set when linking against glibc.

@tiborvass @thaJeztah @cpuguy83 PTAL

@kolyshkin
Revert "homedir: add cgo or osusergo buildtag constraints for unix"
80e338a 
TL;DR: there is no way to do this right.

We do know that in some combination of build tags set (or unset),
linker flags, environment variables, and libc implementation,
this package won't work right. In fact, there is one specific
combination:

1. `CGO_ENABLED=1` (or unset)
2. static binary is being built (e.g. `go build` is run with `-extldflags -static`)
3. `go build` links the binary against glibc
4. `osusergo` is not set

This particular combination results in the following legitimate linker warning:

> cgo_lookup_unix.go: warning: Using 'getpwuid_r' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking

If this warning is ignored and the resulting binary is used on a system
with files from a different glibc version (or without those files), it
could result in a segfault.

The commit being reverted tried to guard against such possibility,
but the problem is, we can only use build tags to account for items
1 and 4 from the above list, while items 2 and 3 do not result in
any build tags being set or unset, making this guard excessive.

Remove it.

This reverts commit 023b072.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Oct 24, 2019 •
edited
Loading

  1. static binary is being built (e.g. go build is run with -extldflags -static)
  2. go build links the binary against glibc

<...>
...while items 2 and 3 do not result in any build tags being set or unset

Let me elaborate on this one.

It's a de-facto convention to set static_build build tag in such case, but it's entirely voluntarily (this might change in the future, see golang/go#26492), so we can't rely on it.

Even if we could, item 3 is still here.

thaJeztah
thaJeztah approved these changes Oct 24, 2019
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@tiborvass
Copy link
Contributor

tiborvass commented Oct 24, 2019

Sorry I don't quite understand this, I need to think this through. Please don't merge yet

@thaJeztah thaJeztah mentioned this pull request Oct 24, 2019
Closed
@kolyshkin
pkg/homedir: clarify Get() docs wrt static linking
7ef475f 
This clarifies comments about static linking made in commit a8608b5.

1. There are two ways to create a static binary, one is to disable
cgo, the other is to set linker flags. When cgo is disabled,
there is no need to use osusergo build tag.

2. osusergo only needs to be set when linking against glibc.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

kolyshkin commented Oct 24, 2019

@tonistiigi PTAL (as per @tiborvass you wanted #39994 in the first place)

@kolyshkin kolyshkin requested a review from tonistiigi October 24, 2019 23:51
@tonistiigi
Copy link
Member

tonistiigi commented Oct 25, 2019

How about we add another build tag that could be enabled for building in musl or dynamic binaries in glibc if osusergo shouldn't be used? It seems risky to allow this panic in one of the default cases if the comment is not seen.

@thaJeztah
Copy link
Member

thaJeztah commented Oct 25, 2019

the panic is in the go std library code; I'm not sure if we should do all that to work around an issue there; any package that uses that would have the same problem

@kolyshkin
Copy link
Contributor Author

kolyshkin commented Oct 25, 2019

How about we add another build tag that could be enabled for building in musl or dynamic binaries in glibc if osusergo shouldn't be used? It seems risky to allow this panic in one of the default cases if the comment is not seen.

So are you suggesting something like i_am_aware_static_linking_has_dark_avenues build tag? I guess that won't work -- people will just set it everywhere without much understanding about the subject.

There's already a warning from a linker, which should not be ignored. We can even make it an error if we add -extldflags -Wl,--fatal-warnings.

@thaJeztah
Copy link
Member

thaJeztah commented Oct 25, 2019

Perhaps we should add that option to make sure we don't make that mistake. For other consumers of this package, I think it's out of scope for us to prevent them from ignoring the warnings

@tiborvass tiborvass merged commit a09e6e3 into moby:master Oct 25, 2019
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Oct 26, 2019
@thaJeztah
bump docker/docker to a09e6e323e55e1a9b21df9c2c555f5668df3ac9b
5dc2f59 
full diff: moby/moby@b6684a4...a09e6e3

relevant changes:

- moby/moby#39995 Update containerd binary to v1.2.10
- moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884)
- moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276)
- moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596)
- moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix"
    - reverts moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix,
      in favor of documenting when to set the `osusergo` build tag. The `osusergo`
      build-flag must be used when compiling a static binary with `cgo` enabled,
      and linking against `glibc`.
- moby/moby#39983 builder: remove legacy build's session handling
  This feature was used by docker build --stream and it was kept experimental.
  Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit.
    - Related: docker#2105 build: remove --stream (was experimental)
- moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty,
  golang/gddo, gorilla/mux
- moby/moby#39713 bump containerd and dependencies to v1.3.0
- moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver
- moby/moby#40070 Use ocischema package instead of custom handler
    - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image
    - relates to docker/hub-feedback#1871
    - relates to distribution/distribution#3024
- moby/moby#39231 Add support for sending down service Running and Desired task counts
- moby/moby#39822 daemon: Use short libnetwork ID in exec-root
- moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion()
    - updates/requires Microsoft/hscshim@2226e083fc390003ae5aa8325c3c92789afa0e7a

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah mentioned this pull request Oct 26, 2019
Merged
3 tasks
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Oct 26, 2019
@thaJeztah
bump docker/docker to a09e6e323e55e1a9b21df9c2c555f5668df3ac9b
7f6cd64 
full diff: moby/moby@b6684a4...a09e6e3

relevant changes:

- moby/moby#39995 Update containerd binary to v1.2.10
- moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884)
- moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276)
- moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596)
- moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix"
    - reverts moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix,
      in favor of documenting when to set the `osusergo` build tag. The `osusergo`
      build-flag must be used when compiling a static binary with `cgo` enabled,
      and linking against `glibc`.
- moby/moby#39983 builder: remove legacy build's session handling
  This feature was used by docker build --stream and it was kept experimental.
  Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit.
    - Related: docker#2105 build: remove --stream (was experimental)
- moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty,
  golang/gddo, gorilla/mux
- moby/moby#39713 bump containerd and dependencies to v1.3.0
- moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver
- moby/moby#40070 Use ocischema package instead of custom handler
    - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image
    - relates to docker/hub-feedback#1871
    - relates to distribution/distribution#3024
- moby/moby#39231 Add support for sending down service Running and Desired task counts
- moby/moby#39822 daemon: Use short libnetwork ID in exec-root
- moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion()
    - updates/requires Microsoft/hscshim@2226e083fc390003ae5aa8325c3c92789afa0e7a

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was referenced Oct 26, 2019
Merged
Merged
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Oct 28, 2019
@thaJeztah
bump docker/docker to a09e6e323e55e1a9b21df9c2c555f5668df3ac9b
ee18946 
full diff: moby/moby@b6684a4...a09e6e3

relevant changes:

- moby/moby#39995 Update containerd binary to v1.2.10
- moby/moby#40001 Update runc to v1.0.0-rc8-92-g84373aaa (CVE-2019-16884)
- moby/moby#39999 bump golang 1.13.1 (CVE-2019-16276)
- moby/moby#40102 bump golang 1.13.3 (CVE-2019-17596)
- moby/moby#40134 Revert "homedir: add cgo or osusergo buildtag constraints for unix"
    - reverts moby/moby#39994 homedir: add cgo or osusergo buildtag constraints for unix,
      in favor of documenting when to set the `osusergo` build tag. The `osusergo`
      build-flag must be used when compiling a static binary with `cgo` enabled,
      and linking against `glibc`.
- moby/moby#39983 builder: remove legacy build's session handling
  This feature was used by docker build --stream and it was kept experimental.
  Users of this endpoint should enable BuildKit anyway by setting Version to BuilderBuildKit.
    - Related: #2105 build: remove --stream (was experimental)
- moby/moby #40045 Bump logrus 1.4.2, go-shellwords, mergo, flock, creack/pty,
  golang/gddo, gorilla/mux
- moby/moby#39713 bump containerd and dependencies to v1.3.0
- moby/moby#39987 Add ability to handle index acknowledgment with splunk log driver
- moby/moby#40070 Use ocischema package instead of custom handler
    - relates to moby/moby#39727 Docker 19.03 doesn't support OCI image
    - relates to docker/hub-feedback#1871
    - relates to distribution/distribution#3024
- moby/moby#39231 Add support for sending down service Running and Desired task counts
- moby/moby#39822 daemon: Use short libnetwork ID in exec-root
- moby/moby#39100 Use Microsoft/hcsshim constants and deprecate pkg/system.GetOsVersion()
    - updates/requires Microsoft/hscshim@2226e083fc390003ae5aa8325c3c92789afa0e7a

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 7f6cd64335dc631efaa8204c01f92aa40939073a
Component: cli
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiborvass tiborvass tiborvass approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@tonistiigi tonistiigi Awaiting requested review from tonistiigi

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@kolyshkin @tiborvass @tonistiigi @thaJeztah