From b5e07e62dd8c7006103085b8d67dfb75f48dc89f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Oct 2022 21:36:17 +0200 Subject: [PATCH] Don't set a default PATH for Windows Commit ecf070a027d7130c89b8a3ef939199775df8a75b updated DefaultPathEnv to return platform specific values, but also defined a default PATH for Windows. However, Windows does not have a default PATH, and the PATH to use must be set by the container image. Also see https://github.com/moby/moby/commit/3c177dc8777e76b4913294d17da4eac30edbd8db, which removed the default PATH on Windows; > This is deliberately empty on Windows as the default path will be set by > the container. Docker has no context of what the default path should be. Signed-off-by: Sebastiaan van Stijn --- client/build_test.go | 16 ++++----- client/llb/exec.go | 4 ++- exporter/containerimage/writer.go | 4 ++- frontend/dockerfile/dockerfile2llb/convert.go | 4 ++- frontend/dockerfile/dockerfile2llb/image.go | 4 ++- frontend/dockerfile/dockerfile_test.go | 2 +- frontend/gateway/container/container.go | 4 ++- solver/llbsolver/ops/exec.go | 4 ++- util/system/path.go | 34 +++++++++++++++---- 9 files changed, 53 insertions(+), 23 deletions(-) diff --git a/client/build_test.go b/client/build_test.go index f6c662d221aa8..fa9b1efcc7621 100644 --- a/client/build_test.go +++ b/client/build_test.go @@ -1363,17 +1363,15 @@ func testClientGatewayContainerPlatformPATH(t *testing.T, sb integration.Sandbox Platform *pb.Platform Expected string }{{ - "default path", - nil, - utilsystem.DefaultPathEnvUnix, + Name: "default path", + Expected: utilsystem.DefaultPathEnvUnix, }, { - "linux path", - &pb.Platform{OS: "linux"}, - utilsystem.DefaultPathEnvUnix, + Name: "linux path", + Platform: &pb.Platform{OS: "linux"}, + Expected: utilsystem.DefaultPathEnvUnix, }, { - "windows path", - &pb.Platform{OS: "windows"}, - utilsystem.DefaultPathEnvWindows, + Name: "windows path", + Platform: &pb.Platform{OS: "windows"}, }} for _, tt := range tests { diff --git a/client/llb/exec.go b/client/llb/exec.go index 457a9b1f2f8e1..f0169146927a4 100644 --- a/client/llb/exec.go +++ b/client/llb/exec.go @@ -161,7 +161,9 @@ func (e *ExecOp) Marshal(ctx context.Context, c *Constraints) (digest.Digest, [] } else if e.constraints.Platform != nil { os = e.constraints.Platform.OS } - env = env.SetDefault("PATH", system.DefaultPathEnv(os)) + if v, ok := system.DefaultPathEnv(os); ok { + env = env.SetDefault("PATH", v) + } } else { addCap(&e.constraints, pb.CapExecMetaSetsDefaultPath) } diff --git a/exporter/containerimage/writer.go b/exporter/containerimage/writer.go index 4e3e678fd7e62..813eb940c9470 100644 --- a/exporter/containerimage/writer.go +++ b/exporter/containerimage/writer.go @@ -674,7 +674,9 @@ func defaultImageConfig() ([]byte, error) { img.Variant = pl.Variant img.RootFS.Type = "layers" img.Config.WorkingDir = "/" - img.Config.Env = []string{"PATH=" + system.DefaultPathEnv(pl.OS)} + if env, ok := system.DefaultPathEnv(pl.OS); ok { + img.Config.Env = []string{"PATH=" + env} + } dt, err := json.Marshal(img) return dt, errors.Wrap(err, "failed to create empty image config") } diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 5352a62622ad8..76e8a5734d4dc 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -628,7 +628,9 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS if d.platform != nil { osName = d.platform.OS } - d.image.Config.Env = append(d.image.Config.Env, "PATH="+system.DefaultPathEnv(osName)) + if env, ok := system.DefaultPathEnv(osName); ok { + d.image.Config.Env = append(d.image.Config.Env, "PATH="+env) + } } // initialize base metadata from image conf diff --git a/frontend/dockerfile/dockerfile2llb/image.go b/frontend/dockerfile/dockerfile2llb/image.go index b6b589e77654c..9b659d8715a05 100644 --- a/frontend/dockerfile/dockerfile2llb/image.go +++ b/frontend/dockerfile/dockerfile2llb/image.go @@ -34,6 +34,8 @@ func emptyImage(platform ocispecs.Platform) dockerspec.DockerOCIImage { img.Variant = platform.Variant img.RootFS.Type = "layers" img.Config.WorkingDir = "/" - img.Config.Env = []string{"PATH=" + system.DefaultPathEnv(platform.OS)} + if env, ok := system.DefaultPathEnv(platform.OS); ok { + img.Config.Env = []string{"PATH=" + env} + } return img } diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 3530b7a191311..d6dbefc08a47d 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -1655,7 +1655,7 @@ COPY Dockerfile . entrypoint []string env []string }{ - {p: "windows/amd64", entrypoint: []string{"cmd", "/S", "/C", "foo bar"}, env: []string{"PATH=c:\\Windows\\System32;c:\\Windows"}}, + {p: "windows/amd64", entrypoint: []string{"cmd", "/S", "/C", "foo bar"}}, {p: "linux/amd64", entrypoint: []string{"/bin/sh", "-c", "foo bar"}, env: []string{"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}}, } { t.Run(exp.p, func(t *testing.T) { diff --git a/frontend/gateway/container/container.go b/frontend/gateway/container/container.go index f34f67c1ca00e..a0905f5d666c2 100644 --- a/frontend/gateway/container/container.go +++ b/frontend/gateway/container/container.go @@ -327,7 +327,9 @@ func (gwCtr *gatewayContainer) Start(ctx context.Context, req client.StartReques if procInfo.Meta.Cwd == "" { procInfo.Meta.Cwd = "/" } - procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "PATH", utilsystem.DefaultPathEnv(gwCtr.platform.OS)) + if env, ok := utilsystem.DefaultPathEnv(gwCtr.platform.OS); ok { + procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "PATH", env) + } if req.Tty { procInfo.Meta.Env = addDefaultEnvvar(procInfo.Meta.Env, "TERM", "xterm") } diff --git a/solver/llbsolver/ops/exec.go b/solver/llbsolver/ops/exec.go index 5f6777592e09e..8dd010e8b4e79 100644 --- a/solver/llbsolver/ops/exec.go +++ b/solver/llbsolver/ops/exec.go @@ -459,7 +459,9 @@ func (e *ExecOp) Exec(ctx context.Context, g session.Group, inputs []solver.Resu if e.platform != nil { currentOS = e.platform.OS } - meta.Env = addDefaultEnvvar(meta.Env, "PATH", utilsystem.DefaultPathEnv(currentOS)) + if env, ok := utilsystem.DefaultPathEnv(currentOS); ok { + meta.Env = addDefaultEnvvar(meta.Env, "PATH", env) + } secretEnv, err := e.loadSecretEnv(ctx, g) if err != nil { diff --git a/util/system/path.go b/util/system/path.go index cdfff68007f5f..ecaa6113c5023 100644 --- a/util/system/path.go +++ b/util/system/path.go @@ -13,16 +13,36 @@ import ( // ':' character . const DefaultPathEnvUnix = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" -// DefaultPathEnvWindows is windows style list of directories to search for -// executables. Each directory is separated from the next by a colon -// ';' character . -const DefaultPathEnvWindows = "c:\\Windows\\System32;c:\\Windows" +// DefaultPathEnvWindows is deliberately empty on Windows and the default path +// must be set by the image. BuildKit has no context of what the default +// path should be. +// +// Deprecated: Windows images must not have a default PATH set. +const DefaultPathEnvWindows = "" -func DefaultPathEnv(os string) string { +// DefaultPathEnv returns the default value for the PATH environment-variable +// if available for the given operating-system. The second return variable +// indicates whether a default exists. +// +// On Linux/Unix, this returns [DefaultPathEnvUnix]. On Windows, no default +// exists, and PATH must not be set as its image-dependent, and must be +// configured through other means. +// +// More details about default PATH values for Windows images can be found +// in the following GitHub discussions: +// +// - [moby/moby#13833] +// - [moby/buildkit#3158] +// - [containerd/containerd#9118] +// +// [moby/moby#13833]: https://github.com/moby/moby/pull/13833 +// [moby/buildkit#3158]: https://github.com/moby/buildkit/pull/3158 +// [containerd/containerd#9118]: https://github.com/containerd/containerd/pull/9118 +func DefaultPathEnv(os string) (string, bool) { if os == "windows" { - return DefaultPathEnvWindows + return "", false } - return DefaultPathEnvUnix + return DefaultPathEnvUnix, true } // NormalizePath cleans the path based on the operating system the path is meant for.