Fix accessing to layer without lock #1351
Conversation
|
I really don't know how to make sure that there are no double locks. |
|
When adding a lock, it is always worth while opening [WIP] PR with this Change in Podman and Buildah to have their tests run against them. |
|
Ok, I'll do it |
layers.go
Outdated
| @@ -563,6 +563,8 @@ func (s *store) newLayerStore(rundir string, layerdir string, driver drivers.Dri | |||
| uidMap: copyIDMap(s.uidMap), | |||
| gidMap: copyIDMap(s.gidMap), | |||
| } | |||
| defer rlstore.Unlock() | |||
| rlstore.RLock() | |||
There was a problem hiding this comment.
IIRC these should be write locks, so that incomplete layers can be cleaned up. (Not in the newROLayerStore case)
warning: I didn’t actually review this carefully — this is just a drive-by comment to make sure it is not forgotten.
There was a problem hiding this comment.
Yes, fixed. FYI: I'm still trying to check it with Podman and Buildah CI.
There was a problem hiding this comment.
Yes, I understand. I just want to make sure, that my change doesn't broke something (e.g. there is no double lock).
When adding a lock, it is always worth while opening [WIP] PR with this Change in Podman and Buildah to have their tests run against them.
tyler92
force-pushed
the
fix-panic-in-layers
branch
from
1868743
to
4211ae4
Compare
There was a possibility to panic due to such behavior: attempted to update last-writer in lockfile without the write lock Fixes: containers#1324 Signed-off-by: Mikhail Khachayants <tyler92@inbox.ru>
tyler92
force-pushed
the
fix-panic-in-layers
branch
from
4211ae4
to
e35b061
Compare
|
CI in Podman and Buildah is green. @mtrmac what do you think about this PR? |
There was a problem hiding this comment.
This LGTM.
For consistency, could you please update containers.go and images.go to do it the same way? They are correct now AFAICS, it’s just a matter of looking the same if they are doing the same thing:
- Lock the lock after creating the whole store object, not before
- Use the store’s
Lock/RLock/Unlockmethod, instead of directly manipulating alockfileobject.
Match containers and image lock to the new layer lock method. in containers#1351 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Match containers and image lock to the new layer lock method. in containers#1351 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
There was a possibility to panic due to such behavior: attempted to update last-writer in lockfile without the write lock
Fixes: #1324
Signed-off-by: Mikhail Khachayants tyler92@inbox.ru