-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Safer use of filepath.EvalSymlinks()
on Windows
#25151
Safer use of filepath.EvalSymlinks()
on Windows
#25151
Conversation
Also I don't think we run any windows/macos unit tests at all in CI. unit tests are only run on linux AFAICT so if we have platform specific test we need to fix that otherwise we are not covered. |
6eb49ee
to
f52d69d
Compare
The behavior of function `path/filepath.EvalSymlinks()` has changed in Go v1.23: - https://go-review.googlesource.com/c/go/+/565136 - https://go.dev/doc/go1.23#minor_library_changes - https://tip.golang.org/doc/godebug As a consequences, starting with Podman 5.3.0, when installing on Windows (WSL) using scoop, Podman fails to start because it fails to find helper binaries. Scoop copies Podman binaries in a folder of type Junction and `EvalSymlinks` returns an error. The problem is described in containers#24557. To address this problem we are checking if a path is a `Symlink` before calling `EvalSymlinks` and, if it's not (hardlinks, mount points or canonical files), we are calling `path/filepath.Clean` for consistency. In fact `path/filepath.EvalSymlinks`, after evaluating a symlink target, calls `Clean` too. Signed-off-by: Mario Loriedo <[email protected]>
f52d69d
to
513b4aa
Compare
I think you are right. If you don't mind, I would not fix it in this PR, though Also, Windows builds are run on Linux too (CI doesn't use |
Yes that is fine, I wasn't trying to stay that you should fix that right now just that we have a testing gap for platform specific unit tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: l0rd, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
LGTM |
/lgtm |
/cherry-pick v5.4 |
@l0rd: new pull request created: #25156 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
The behavior of function
path/filepath.EvalSymlinks()
has changed in Go v1.23:As a consequence, starting with Podman 5.3.0, when installing on Windows (WSL) using Scoop, Podman fails to start because it fails to find helper binaries. Scoop copies Podman binaries in a folder of type Junction and
EvalSymlinks
returns an error. The problem is described in #24557.To address this problem, we check if a path is a
Symlink
before callingEvalSymlinks
, and if it's not (hard links, mount points, or canonical files), we callpath/filepath.Clean
for consistency. In fact,path/filepath.EvalSymlinks
, after evaluating a path, callsClean
, too.Fixes #24557
Does this PR introduce a user-facing change?