driver/overlay: fix concurrent map write on stagingDirsLocks #2300
Conversation
Seen in podman CI, a map by default in not concurrent safe. As the driver can be used in parallel by callers we must guard against this here. Fixes containers#2297 Signed-off-by: Paul Holzinger <pholzing@redhat.com>
That should be more memory efficient as we do not have to allocate a new map. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, giuseppe, 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 |
| @@ -130,6 +130,9 @@ type Driver struct { | |||
| usingMetacopy bool | |||
| usingComposefs bool | |||
|
|
|||
| stagingDirsLocksMutex *sync.Mutex | |||
There was a problem hiding this comment.
For future reference, I think it’s a bit more idiomatic not to use pointers for Mutex fields — that way linters can warn about unexpectedly making copies of the containing struct.
There was a problem hiding this comment.
When Driver is copied it also copy the same stagingDirsLocks map, as such it should copy the same Mutex which is why I used the pointer as it will keep pointing to the same Mutex.
Maybe that is just an invalid pattern which we cannot support anyway. Looking at this again we only return interfaces and not the type so it should be impossible for callers to use Driver directly to copy so I think you are right we can remove the extra pointer redirection here and rely on the linters to catch misuse
There was a problem hiding this comment.
As you say, in this case, things are perfectly fine: the protected stagingDirsLocks, as a map, is implicitly a reference type — so if something did make a copy, both copies would point to the same lock and the same map; the map would be protected and accesses should not crash (whether such uses would mean anything useful is a different matter).
We 100% don’t support copying the Driver struct. (As usual, Go doesn’t let us explicitly express that.) So this is all pedantic anyway.
- Actually, the supposedly-recommended/idiomatic way to express “don’t copy this type” is to add a mutex (-like method). Compare
Line 223 in 7595f18
There was a problem hiding this comment.
Actually, the supposedly-recommended/idiomatic way to express “don’t copy this type” is to add a mutex (-like method). Compare
LOL, I keep the rewrite in rust comments to myself.
Seen in podman CI, a map by default in not concurrent safe. As the
driver can be used in parallel by callers we must guard against this
here.
Fixes #2297
drivers/overlay: use clear() for stagingDirsLocks
That should be more memory efficient as we do not have to allocate a new map.