Skip to content
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

deps: update to github.com/cyphar/[email protected] #4549

Merged
merged 1 commit into from
Dec 7, 2024

Conversation

cyphar
Copy link
Member

@cyphar cyphar commented Dec 6, 2024

This fixes a regression in use of securejoin.MkdirAll, where multiple
runc processes racing to create the same mountpoint in a shared rootfs
would result in spurious EEXIST errors. In particular, this regression
caused issues with BuildKit.

Fixes: dd827f7 ("utils: switch to securejoin.MkdirAllHandle")
Fixes #4543
Signed-off-by: Aleksa Sarai [email protected]

This fixes a regression in use of securejoin.MkdirAll, where multiple
runc processes racing to create the same mountpoint in a shared rootfs
would result in spurious EEXIST errors. In particular, this regression
caused issues with BuildKit.

Fixes: dd827f7 ("utils: switch to securejoin.MkdirAllHandle")
Signed-off-by: Aleksa Sarai <[email protected]>
@cyphar cyphar added the backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 label Dec 6, 2024
@cyphar cyphar added this to the 1.2.3 milestone Dec 6, 2024
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM (also reviewed cyphar/filepath-securejoin#35, which also LGTM).

@lifubang lifubang merged commit fe371b9 into opencontainers:main Dec 7, 2024
40 checks passed
@lifubang lifubang added backport/1.2-done A PR in main branch which has been backported to release-1.2 and removed backport/1.2-todo A PR in main branch which needs to be backported to release-1.2 labels Dec 7, 2024
@cyphar cyphar deleted the racing-mkdirall branch December 7, 2024 17:38
@lifubang
Copy link
Member

lifubang commented Dec 8, 2024

I think we can make v1.2.3 release now!

Another thing, maybe we also need to backport to release-1.1, because it's also a regression in that branch, and we have not announced EOL of v1.1 yet.
Maybe we need to add a RELEASE.md to announce our release policy?

@cyphar
Copy link
Member Author

cyphar commented Dec 8, 2024

@lifubang 1.1.x doesn't use securejoin.MkdirAll, it still uses the hotfix implementation which doesn't have this exact regression. Looking at the implementation though, there is a very small race window where you could see this behaviour (if the directory is created after the first openat but before the mkdir). We could fix it, but it seems quite unlikely that you'll hit the issue in practice (given that this behaviour was only noticed in 1.2.x) and I don't think it's worth doing another 1.1.x release just for this.

Regarding RELEASE.md, I sent an email to all the maintainers a few months ago to try to get some agreement on the policy we want before we write it down. I can open a PR with my proposal if you like.

@lifubang
Copy link
Member

lifubang commented Dec 8, 2024

I can open a PR with my proposal if you like.

Yes, thanks.

@tonistiigi
Copy link
Contributor

1.1.x doesn't use securejoin.MkdirAll, it still uses the hotfix implementation which doesn't have this exact regression. Looking at the implementation though, there is a very small race window where you could see this behaviour (if the directory is created after the first openat but before the mkdir). We could fix it, but it seems quite unlikely that you'll hit the issue in practice (given that this behaviour was only noticed in 1.2.x) and I don't think it's worth doing another 1.1.x release just for this.

FWIW I did reproduce this in 1.1.14 as well with steps from #4543 . I didn't notice any meaningful difference in how often it appeared.

@cyphar
Copy link
Member Author

cyphar commented Dec 11, 2024

Hmmm, that's odd. I would've expected the race window to be much smaller...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.2-done A PR in main branch which has been backported to release-1.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Starting parallel containers with same mounts can cause an error
5 participants