solver: fix possible concurrent map access on cache export#4346
solver: fix possible concurrent map access on cache export#4346jedevc merged 1 commit intomoby:masterfrom
Conversation
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
6f5aa4b to
e99bfa9
Compare
|
@tonistiigi @jedevc Technically probably not the right people to reach out to, but I've been keeping my eyes on the Docker Engine releases for this fix, because it (and #4157) has blocked our adoption of Docker 24.0.7 thus far. But the 24.0.8 release doesn't seem to be happening anytime soon (the last patch-level release was months ago, https://docs.docker.com/engine/release-notes/24.0/). If you have any good contacts within Docker Inc, could you ask them kindly if there is a schedule for when we can expect the 24.0.8 release? Thanks in advance! 🙏 |
|
docker 24.0 uses BuildKit 0.11, which doesn't have this patch. It won't be updated to v0.12, as that's a major update (for better words, as BuildKit "major" updates are "minor" updates in SemVer). Docker 25.0 will be released soon, and should have this patch (not sure if the changes in this PR can be backported to / apply to v0.11) |
|
Thanks for the info @thaJeztah, appreciated. 👍 I'll be keeping my eyes open for Docker 25.0 then. |
|
Ah, it's in fact released already: https://docs.docker.com/engine/release-notes/25.0/ |
fix #2458
fix #4305
I'm not super happy with this as ideally, we could guarantee the immutability of the cache keys before export and do a better job of avoiding leaks between
cacheManageronly used by specific build requests.This adds some missing locks to the changing
idsmap. The rest of the places accessingidsshould have exclusive access to the struct already.For the export path and
combinedManagerI make a copy of the map before to avoid holding the lock for the whole iteration and make sure no new cache managers are added in the middle of the function. Moving other (nonids)e.kaccesses to localkvariable is just for better readability/consitency and does not fix any race case.@jedevc