containers / storage

Container Storage Library
Apache License 2.0
556 stars 237 forks source link

Reexec causes infinite loop, if built without CGO_ENABLED #1435

Open hesch opened 1 year ago

hesch commented 1 year ago

Recently I opened this issue https://github.com/containers/buildah/issues/4370 in the buildah repo and found the culprit. While building, I didn't specify CGO_ENABLED=1. This causes an infinite loop of forks, because this is never called: https://github.com/containers/storage/blob/89878da4a1548918d0f0a1f0ba535e68d7dd11f7/pkg/unshare/unshare_cgo.go#L6-L11 Is it possible to check for this while compiling and failing the build?

Steps to reproduce: main.go:

package main

import (
        "github.com/containers/storage/pkg/unshare"
        "github.com/containers/storage/pkg/reexec"
)

func main() {
        reexec.Init()
        unshare.MaybeReexecUsingUserNamespace(false)
}

Compile this with CGO_ENABLED=0 go build -o main main.go

=> Executing the binary leads to an infinite loop (and is akin to a fork bomb)

rhatdan commented 1 year ago

@giuseppe @nalind Is there a simple way to make this a noop if CGO not available?

hesch commented 1 year ago

I would prefer the build failing or an error message to be printed instead of it being a noop. If it is a noop something somewhere down the line will fail and the error message will not lead the user to the root cause of the problem.

giuseppe commented 1 year ago

if the code in unshare_cgo.go is not used, then we have a version in Go in unshare_linux.go. We'd have to understand why it is not running and causing a new reexec

giuseppe commented 1 year ago

@hesch isn't the code in unshare_linux.go not running in your case before the reexec happens?

giuseppe commented 1 year ago

we could probably use go exec to create the user namespace instead of the child doing the unshare(CLONE_NEWUSER) but it is not a trivial fix. The easiest is probably to add a new file unshare_nocgo.go and set a variable there saying cgo is not available so MaybeReexecUsingUserNamespace can fail with an useful error message

hesch commented 1 year ago

@giuseppe I was also pretty confused at first how this is even supposed to work. There exists no implementation in pure go. It needs the C-Code to call unshare(CLONE_NEWUSER). What I don't understand is why unshare_linux.go does not use Cmd.SysProcAttr.UnshareFlags to just do the unshare while the new process is cloned. Wouldn't that be much easier?

giuseppe commented 1 year ago

@hesch I agree, that should be easier than creating the namespaces directly from the new process

rhatdan commented 1 year ago

@hesch @giuseppe Interested in opening a PR?

hesch commented 1 year ago

What would you say is the best solution here? We could

  1. Fail the build if cgo is not enabled as suggested initially This has the advantage of potentially fixing other bugs that occur if cgo is not available.
  2. Fix the code so that it no longer requires cgo Here we have two options: a. Just port the C code as is to go This should be compatible with all OSes? b. Implement a new solution using clone This will drop support for all systems without the clone syscall. I have no Idea if BSD and darwin implement this syscall.