fix: use unix path separator since windows path already normalized #4825

profnandaa · 2024-04-05T11:41:00Z

In the case for Windows, this line at frontend/dockerfile/dockerfile2llb/convert.go#L1142 was adding the \\ to a path that is already normalized to unix-format, hence ending up with dest paths like: /\\ for C:\\ and /test\\ for C:\\test\\.

buildkit/frontend/dockerfile/dockerfile2llb/convert.go

Line 1142 in fb97e6d

dest += string(filepath.Separator)

The src paths are well normalized too at ~L1290:

buildkit/frontend/dockerfile/dockerfile2llb/convert.go

Line 1287 in fb97e6d

src, err = system.NormalizePath("/", src, d.platform.OS, false)

This change removes the block of code and instead does the "/" appending using the keepSlash logic
that is in system.NormalizePath called in pathRelativeToWorkingDir function before.

Additionally in solver/llbsolver/file/backend.go, the error message is enhanced to specify "destination" path, since "source" path is also specified in it's error message. This was the "cleaning path" wrapped error reported in issue #4696 :

error: failed to solve: cleaning path: removing drive letter: UNC paths are not supported

fixes #4696

tonistiigi

I think the intent could have been that the llb.File functions already expect the paths to be in unix-style. Is there some calls that don't do that? Eg. look at system.NormalizePath.

profnandaa · 2024-04-08T07:12:17Z

I think the intent could have been that the llb.File functions already expect the paths to be in unix-style. Is there some calls that don't do that? Eg. look at system.NormalizePath.

From your previous comments here with Gabriel #3908, looks like that was the intention; and it was to be a stripped-down version of system.NormalizePath -- I notice that system.NormalizePath is doing a custom toSlash, perhaps I should take the same approach?

Also, I'm trying to understand how this edge-case isn't covered. This is only happening for the root directory, something like COPY hello.txt C:\\test\\ works okay, though it's st.action.dest is "/test\\":

> github.com/moby/buildkit/client/llb.(*FileOp).Marshal() C:/dev/container-core/buildkit/client/llb/fileop.go:775 (hits goroutine(450):1 total:1) (PC: 0x125029c)
Warning: listing may not match stale executable
   770:                         output = 0
   771:                 }
   772:
   773:                 var parent string
   774:                 if st.fa.state != nil {
=> 775:                         parent, err = st.fa.state.GetDir(ctx)
   776:                         if err != nil {
   777:                                 return "", nil, nil, nil, err
   778:                         }
   779:                 }
   780:
(dlv) p st.action.dest
"/\\"
(dlv) bt
0  0x000000000125029c in github.com/moby/buildkit/client/llb.(*FileOp).Marshal
   at C:/dev/container-core/buildkit/client/llb/fileop.go:775
1  0x0000000001264917 in github.com/moby/buildkit/client/llb.marshal
   at C:/dev/container-core/buildkit/client/llb/state.go:201
2  0x0000000001263bb6 in github.com/moby/buildkit/client/llb.State.Marshal
   at C:/dev/container-core/buildkit/client/llb/state.go:145
3  0x0000000001eaaadf in github.com/moby/buildkit/frontend/dockerfile/builder.Build.func5
   at C:/dev/container-core/buildkit/frontend/dockerfile/builder/build.go:130
4  0x0000000001ba1b90 in github.com/moby/buildkit/frontend/dockerui.(*Client).Build.func1
   at C:/dev/container-core/buildkit/frontend/dockerui/build.go:39
5  0x0000000000d20e69 in golang.org/x/sync/errgroup.(*Group).Go.func1
   at C:/dev/container-core/buildkit/vendor/golang.org/x/sync/errgroup/errgroup.go:75
6  0x00000000004e25a1 in runtime.goexit
   at C:/Program Files/Go/src/runtime/asm_amd64.s:1695
(dlv)

Is there anywhere up the stack that the cleaning is supposed to happen? Will appreciate some pointers.

profnandaa · 2024-04-08T08:01:50Z

@tonistiigi -- Found it, the path was being normalized but this one line was breaking it by adding a Windows suffix. So, you were right, the expectation is that once we get at llb, the paths are are in unix format.

buildkit/frontend/dockerfile/dockerfile2llb/convert.go

Line 1142 in fb97e6d

dest += string(filepath.Separator)

TBBle · 2024-04-08T11:37:34Z

It's not related to this code, but it's nearby: Is line 1126 faulty, should llb.Platform(*d.platform) be llb.Platform(platform)? (I was looking because this means the issues I was seeing with workdir years ago were actually unrelated, since this fix is directly in dispatchCopy, and I was wondering if the same issue was present in dispatchWorkdir)

profnandaa · 2024-04-08T12:36:47Z

It's not related to this code, but it's nearby: Is line 1126 faulty, should llb.Platform(*d.platform) be llb.Platform(platform)? (I was looking because this means the issues I was seeing with workdir years ago were actually unrelated, since this fix is directly in dispatchCopy, and I was wondering if the same issue was present in dispatchWorkdir)

@TBBle -- no, WORKDIR is now handled correctly. With the following lines:

buildkit/frontend/dockerfile/dockerfile2llb/convert.go

Line 1094 in fb97e6d

    
           wd, err := system.NormalizeWorkdir(d.image.Config.WorkingDir, c.Path, d.platform.OS)

buildkit/frontend/dockerfile/dockerfile2llb/convert.go

Lines 1104 to 1105 in fb97e6d

    
           // From this point forward, we can use UNIX style paths. 
        
           wd = system.ToSlash(wd, d.platform.OS)

and *d.platform is the same as platform at that line:

> github.com/moby/buildkit/frontend/dockerfile/dockerfile2llb.dispatchWorkdir() C:/dev/container-core/buildkit/frontend/dockerfile/dockerfile2llb/convert.go:1123 (PC: 0x1ad1e87)
  1118:                         }
  1119:                         env, err := d.state.Env(context.TODO())
  1120:                         if err != nil {
  1121:                                 return err
  1122:                         }
=>1123:                         d.state = d.state.File(llb.Mkdir(wd, 0755, mkdirOpt...),
  1124:                                 llb.WithCustomName(prefixCommand(d, uppercaseCmd(processCmdEnv(opt.shlex, c.String(), env)), d.prefixPlatform, &platform, env)),
  1125:                                 location(opt.sourceMap, c.Location()),
  1126:                                 llb.Platform(*d.platform),
  1127:                         )
  1128:                         withLayer = true
(dlv) p *d.platform
github.com/opencontainers/image-spec/specs-go/v1.Platform {
        Architecture: "amd64",
        OS: "windows",
        OSVersion: "10.0.22621",
        OSFeatures: []string len: 0, cap: 0, nil,
        Variant: "",}
(dlv) p platform
github.com/opencontainers/image-spec/specs-go/v1.Platform {
        Architecture: "amd64",
        OS: "windows",
        OSVersion: "10.0.22621",
        OSFeatures: []string len: 0, cap: 0, nil,
        Variant: "",}

Testing with both:

WORKDIR C:\\
# and
WORKDIR C:\\test

both resolve correctly to wd = / and /test respectively.

Thanks for checking it out for verification!

tonistiigi · 2024-04-08T22:43:18Z

frontend/dockerfile/dockerfile2llb/convert.go

@@ -1139,7 +1139,10 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error {
 	}

 	if cfg.params.DestPath == "." || cfg.params.DestPath == "" || cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator {


Would it be bit more correct if this condition also checked:

lastChar := cfg.params.DestPath[len(cfg.params.DestPath)-1] isWindows := if cfg.params.DestPath == "." || cfg.params.DestPath == "" || lastChar == "/" || (lastChar == "\" && isWindows) {

BTW, given what pathRelativeToWorkingDir is doing at L1136 (normalizing the path cfg.params.DestPath already) , do we actually need this block now or we just continue with dest from here onwards?

buildkit/frontend/dockerfile/dockerfile2llb/convert.go

Lines 1550 to 1568 in fb97e6d

func pathRelativeToWorkingDir(s llb.State, p string, platform ocispecs.Platform) (string, error) {

dir, err := s.GetDir(context.TODO(), llb.Platform(platform))

if err != nil {

return "", err

}

if len(p) == 0 {

return dir, nil

}

p, err = system.CheckSystemDriveAndRemoveDriveLetter(p, platform.OS)

if err != nil {

return "", errors.Wrap(err, "removing drive letter")

}

if system.IsAbs(p, platform.OS) {

return system.NormalizePath("/", p, platform.OS, false)

}

return system.NormalizePath(dir, p, platform.OS, false)

}

Afaics, pathRelativeToWorkingDir calls system.NormalizePath() and NormalizePath() takes keepSlash bool, but pathRelativeToWorkingDir is setting it to false. If we would set it to true instead then it looks like we don't need this extra block.

cc @gabriel-samfira to double-check the intention of keepSlash=false

Perhaps this was for some old code, since? there's no where system.NormalizePath() is called with keepSlash=true, as far as I can see. At some point, may be we should do away with that param?

$ grep -rn "\.NormalizePath(" frontend/dockerfile/dockerfile2llb/convert.go:1279: pattern, err = system.NormalizePath("/", pattern, d.platform.OS, false) frontend/dockerfile/dockerfile2llb/convert.go:1287: src, err = system.NormalizePath("/", src, d.platform.OS, false) frontend/dockerfile/dockerfile2llb/convert.go:1565: return system.NormalizePath("/", p, platform.OS, false) frontend/dockerfile/dockerfile2llb/convert.go:1567: return system.NormalizePath(dir, p, platform.OS, false)

I think I would rather keep keepSlash or add a new helper function for this functionality (if there is only one caller then it can remain private). The current long if statement is not very robust, can't be tested and is easy to break.

the keepSlash logic was carried forward from the function that NormalizePath() replaced.

Looking over the rest of the PR now.

I think it's safe to switch NormalizePath to true in pathRelativeToWorkingDir. That function is only used once, in convert.go. And we seem to be duplicating some of its logic in that if statement that follows right after we call pathRelativeToWorkingDir(). Let's just remove that if block and flip that boolean in pathRelativeToWorkingDir

Thank @gabriel-samfira for the review. I've made the change, PTAL.

TBBle · 2024-04-09T04:09:43Z

and *d.platform is the same as platform at that line:

Okay, so d.platform cannot be nil here as we unconditionally access d.platform.OS earlier? In that case lines 1115-1117 (setting platform) appear useless. (On my phone, so may be misreading the code...)

profnandaa · 2024-04-09T04:48:14Z

and *d.platform is the same as platform at that line:

Okay, so d.platform cannot be nil here as we unconditionally access d.platform.OS earlier? In that case lInes 1115-1117 (setting platform) appear useless. (On my phone, so may be misreading the code...)

@TBBle -- oh, my bad, that's right. llb.Platform(platform) will be a better defensive approach. @tonistiigi -- can I add this as part of this PR and expand it's scope to fixing copy and workdir dispatchs?

In the case for Windows, this line at frontend/dockerfile/dockerfile2llb/convert.go#L1142 ```go dest += string(filepath.Separator) ``` was adding the `\\` to a path that is already normalized to unix-format, hence ending up with dest paths like `/\\` for `C:\\` and `/test\\` for `C:\\test\\`. the src paths are well normalized too at ~L1290. This change removes the block of code and instead does the "/" appending using the keepSlash logic that is in system.NormalizePath called in pathRelativeToWorkingDir() function before. fixes moby#4696 Signed-off-by: Anthony Nandaa <[email protected]>

profnandaa · 2024-04-11T15:29:03Z

and *d.platform is the same as platform at that line:

Okay, so d.platform cannot be nil here as we unconditionally access d.platform.OS earlier? In that case lInes 1115-1117 (setting platform) appear useless. (On my phone, so may be misreading the code...)

@TBBle -- oh, my bad, that's right. llb.Platform(platform) will be a better defensive approach. @tonistiigi -- can I add this as part of this PR and expand it's scope to fixing copy and workdir dispatchs?

Let me make this fix as a separate PR @TBBle

In my previous fix on moby#4825, I had removed this line knowing that all that had been addressed in `pahtRelativeToWorkingDir`: ```go if cfg.params.DestPath == "." // <-- this one || cfg.params.DestPath == "" || cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator { dest += string(filepath.Separator) } ``` However, I had overlooked the `"."` scenario. `""`, `"/"` are all handled correctly in `system.NormalizePath()`. This change therefore undoes this, to make sure "." is transformed correctly to "./" during COPY operation. Signed-off-by: Anthony Nandaa <[email protected]>

In my previous fix on moby#4825, I had removed this line knowing that all that had been addressed in `pahtRelativeToWorkingDir`: ```go if cfg.params.DestPath == "." // <-- this one || cfg.params.DestPath == "" || cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator { dest += string(filepath.Separator) } ``` However, I had overlooked the `"."` scenario. `""`, `"/"` are all handled correctly in `system.NormalizePath()`. This change therefore undoes this, to make sure "." is transformed correctly to "./" during COPY operation. fixes moby#5070 Signed-off-by: Anthony Nandaa <[email protected]>

In my previous fix on moby#4825, I had removed this line knowing that all that had been addressed in `pahtRelativeToWorkingDir`: ```go if cfg.params.DestPath == "." // <-- this one || cfg.params.DestPath == "" || cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator { dest += string(filepath.Separator) } ``` However, I had overlooked the `"."` and `""` scenarios. `""`. The `"/"`case is handled correctly in `system.NormalizePath()`. This change therefore undoes this, to make sure "." is transformed correctly to "./" during COPY operation. fixes moby#5070 Signed-off-by: Anthony Nandaa <[email protected]>

In my previous fix on moby#4825, I had removed this line knowing that all that had been addressed in `pahtRelativeToWorkingDir`: ```go if cfg.params.DestPath == "." // <-- this one || cfg.params.DestPath == "" || cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator { dest += string(filepath.Separator) } ``` However, I had overlooked the `"."` and `""` scenarios. `""`. The `"/"`case is handled correctly in `system.NormalizePath()`. This change therefore undoes this, to make sure "." is transformed correctly to "./" during COPY operation, same to "" -> "/". fixes moby#5070 Signed-off-by: Anthony Nandaa <[email protected]>

In my previous fix on moby#4825, I had removed this line knowing that all that had been addressed in `pahtRelativeToWorkingDir`: ```go if cfg.params.DestPath == "." // <-- this one || cfg.params.DestPath == "" || cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator { dest += string(filepath.Separator) } ``` However, I had overlooked the `"."` and `""` scenarios. `""`. The `"/"`case is handled correctly in `system.NormalizePath()`. This change therefore undoes this, to make sure "." is transformed correctly to "./" during COPY operation, same to "" -> "./". This is important especially for WORKDIR that are not `/`, so that `COPY --link` operations are handled properly. fixes moby#5070 Signed-off-by: Anthony Nandaa <[email protected]>

profnandaa marked this pull request as ready for review April 5, 2024 11:41

profnandaa requested a review from tonistiigi April 5, 2024 11:44

profnandaa added the area/windows-wcow label Apr 5, 2024

profnandaa requested review from jedevc and crazy-max April 5, 2024 11:53

jedevc requested a review from gabriel-samfira April 5, 2024 12:48

tonistiigi reviewed Apr 5, 2024

View reviewed changes

profnandaa force-pushed the fix-4696-normalize-path-function branch from 520d30f to 54261cb Compare April 8, 2024 08:09

profnandaa changed the title ~~fix: normalizePath function to standardize the paths correctly~~ fix: use unix path separator since windows path already normalized Apr 8, 2024

profnandaa force-pushed the fix-4696-normalize-path-function branch from 54261cb to 02c144e Compare April 8, 2024 09:38

tonistiigi reviewed Apr 8, 2024

View reviewed changes

profnandaa force-pushed the fix-4696-normalize-path-function branch from 02c144e to 018155f Compare April 11, 2024 15:27

tonistiigi approved these changes Apr 11, 2024

View reviewed changes

gabriel-samfira approved these changes Apr 11, 2024

View reviewed changes

tonistiigi merged commit 76c7422 into moby:master Apr 11, 2024
72 checks passed

profnandaa deleted the fix-4696-normalize-path-function branch May 15, 2024 07:18

tonistiigi mentioned this pull request Jun 21, 2024

[v0.14] dockerfile: mkdir xxxx not a directory - with COPY --link #5070

Closed

profnandaa mentioned this pull request Jun 24, 2024

fix: dot and empty paths normalized correctly for COPY to WORKDIR #5080

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use unix path separator since windows path already normalized #4825

fix: use unix path separator since windows path already normalized #4825

profnandaa commented Apr 5, 2024 •

edited

Loading

tonistiigi left a comment

profnandaa commented Apr 8, 2024

profnandaa commented Apr 8, 2024 •

edited

Loading

TBBle commented Apr 8, 2024

profnandaa commented Apr 8, 2024

tonistiigi Apr 8, 2024

profnandaa Apr 9, 2024 •

edited

Loading

tonistiigi Apr 9, 2024

profnandaa Apr 9, 2024

tonistiigi Apr 9, 2024

gabriel-samfira Apr 11, 2024

gabriel-samfira Apr 11, 2024

profnandaa Apr 11, 2024 •

edited

Loading

TBBle commented Apr 9, 2024 •

edited

Loading

profnandaa commented Apr 9, 2024

profnandaa commented Apr 11, 2024

		@@ -1139,7 +1139,10 @@ func dispatchCopy(d *dispatchState, cfg copyConfig) error {
		}

		if cfg.params.DestPath == "." \|\| cfg.params.DestPath == "" \|\| cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator {

	func pathRelativeToWorkingDir(s llb.State, p string, platform ocispecs.Platform) (string, error) {
	dir, err := s.GetDir(context.TODO(), llb.Platform(platform))
	if err != nil {
	return "", err
	}

	if len(p) == 0 {
	return dir, nil
	}
	p, err = system.CheckSystemDriveAndRemoveDriveLetter(p, platform.OS)
	if err != nil {
	return "", errors.Wrap(err, "removing drive letter")
	}

	if system.IsAbs(p, platform.OS) {
	return system.NormalizePath("/", p, platform.OS, false)
	}
	return system.NormalizePath(dir, p, platform.OS, false)
	}

fix: use unix path separator since windows path already normalized #4825

fix: use unix path separator since windows path already normalized #4825

Conversation

profnandaa commented Apr 5, 2024 • edited Loading

tonistiigi left a comment

Choose a reason for hiding this comment

profnandaa commented Apr 8, 2024

profnandaa commented Apr 8, 2024 • edited Loading

TBBle commented Apr 8, 2024

profnandaa commented Apr 8, 2024

tonistiigi Apr 8, 2024

Choose a reason for hiding this comment

profnandaa Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

tonistiigi Apr 9, 2024

Choose a reason for hiding this comment

profnandaa Apr 9, 2024

Choose a reason for hiding this comment

tonistiigi Apr 9, 2024

Choose a reason for hiding this comment

gabriel-samfira Apr 11, 2024

Choose a reason for hiding this comment

gabriel-samfira Apr 11, 2024

Choose a reason for hiding this comment

profnandaa Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

TBBle commented Apr 9, 2024 • edited Loading

profnandaa commented Apr 9, 2024

profnandaa commented Apr 11, 2024

profnandaa commented Apr 5, 2024 •

edited

Loading

profnandaa commented Apr 8, 2024 •

edited

Loading

profnandaa Apr 9, 2024 •

edited

Loading

profnandaa Apr 11, 2024 •

edited

Loading

TBBle commented Apr 9, 2024 •

edited

Loading