Skip to content

deps: update to github.com/cyphar/filepath-securejoin@v0.3.5#4549

Merged
lifubang merged 1 commit intoopencontainers:mainfrom
cyphar:racing-mkdirall
Dec 7, 2024
Merged

deps: update to github.com/cyphar/filepath-securejoin@v0.3.5#4549
lifubang merged 1 commit intoopencontainers:mainfrom
cyphar:racing-mkdirall

Conversation

@cyphar
Copy link
Copy Markdown
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 cyphar@cyphar.com

@cyphar
deps: update to github.com/cyphar/filepath-securejoin@v0.3.5
2f1b662 
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 <cyphar@cyphar.com>
@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
kolyshkin
kolyshkin approved these changes Dec 6, 2024
View reviewed changes
Copy link
Copy Markdown
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
@cyphar cyphar mentioned this pull request Dec 7, 2024
Merged
@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
Copy Markdown
Member

lifubang commented Dec 8, 2024 •
edited
Loading

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
Copy Markdown
Member Author

cyphar commented Dec 8, 2024 •
edited
Loading

@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
Copy Markdown
Member

lifubang commented Dec 8, 2024

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

Yes, thanks.

@kolyshkin kolyshkin removed this from the 1.2.3 milestone Dec 9, 2024
@lifubang lifubang mentioned this pull request Dec 10, 2024
Closed
@tonistiigi
Copy link
Copy Markdown
Contributor

tonistiigi commented Dec 11, 2024

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
Copy Markdown
Member Author

cyphar commented Dec 11, 2024

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

dkoshkin added a commit to mesosphere/konvoy-image-builder that referenced this pull request Apr 17, 2025
@dkoshkin
fix: update Containerd to pull runc race fix
770ab1a 
The bug was introduced in a library bump opencontainers/runc#4393
on Sep 11th, 2024

Then it was fixed in opencontainers/runc#4549
on Dec 6th, 2024

During that time, Konvoy was always using the main branch when building runc for Containerd tars.
containerd-1.6.33 used in a previous minor release would not have had this bug,
but containerd-1.7.22 did introduce it.

containerd-1.7.24 built on Dec 11 has the fix
```
containerd-1.6.28-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Fri Sep 13 12:30:10 2024, from Unix, original size modulo 2^32 155238400
containerd-1.6.33-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Wed Jun 26 21:16:52 2024, from Unix, original size modulo 2^32 149504000
containerd-1.7.22-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Tue Sep 24 22:17:43 2024, from Unix, original size modulo 2^32 152832000
containerd-1.7.24-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Wed Dec 11 22:13:07 2024, from Unix, original size modulo 2^32 153128960
```
@dkoshkin dkoshkin mentioned this pull request Apr 17, 2025
Merged
dkoshkin added a commit to mesosphere/konvoy-image-builder that referenced this pull request Apr 17, 2025
@dkoshkin
fix: update Containerd to pull runc race fix
5d36ca8 
The bug was introduced in a library bump opencontainers/runc#4393
on Sep 11th, 2024

Then it was fixed in opencontainers/runc#4549
on Dec 6th, 2024

During that time, Konvoy was always using the main branch when building runc for Containerd tars.
containerd-1.6.33 used in a previous minor release would not have had this bug,
but containerd-1.7.22 did introduce it.

containerd-1.7.24 built on Dec 11 has the fix
```
containerd-1.6.28-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Fri Sep 13 12:30:10 2024, from Unix, original size modulo 2^32 155238400
containerd-1.6.33-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Wed Jun 26 21:16:52 2024, from Unix, original size modulo 2^32 149504000
containerd-1.7.22-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Tue Sep 24 22:17:43 2024, from Unix, original size modulo 2^32 152832000
containerd-1.7.24-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Wed Dec 11 22:13:07 2024, from Unix, original size modulo 2^32 153128960
```
dkoshkin added a commit to mesosphere/konvoy-image-builder that referenced this pull request Apr 17, 2025
@dkoshkin
fix: update Containerd to pull runc race fix (#1293)
baf4dc9 
The bug was introduced in a library bump opencontainers/runc#4393
on Sep 11th, 2024

Then it was fixed in opencontainers/runc#4549
on Dec 6th, 2024

During that time, Konvoy was always using the main branch when building runc for Containerd tars.
containerd-1.6.33 used in a previous minor release would not have had this bug,
but containerd-1.7.22 did introduce it.

containerd-1.7.24 built on Dec 11 has the fix
```
containerd-1.6.28-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Fri Sep 13 12:30:10 2024, from Unix, original size modulo 2^32 155238400
containerd-1.6.33-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Wed Jun 26 21:16:52 2024, from Unix, original size modulo 2^32 149504000
containerd-1.7.22-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Tue Sep 24 22:17:43 2024, from Unix, original size modulo 2^32 152832000
containerd-1.7.24-d2iq.1-rhel-8.6-x86_64.tar.gz: gzip compressed data, last modified: Wed Dec 11 22:13:07 2024, from Unix, original size modulo 2^32 153128960
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lifubang lifubang lifubang approved these changes

@kolyshkin kolyshkin kolyshkin approved these changes

+2 more reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@austinvazquez austinvazquez austinvazquez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport/1.2-done A PR in main branch which has been backported to release-1.2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Starting parallel containers with same mounts can cause an error

5 participants

@cyphar @lifubang @tonistiigi @kolyshkin @austinvazquez