opensource/podman
1
0
Fork 0
mirror of https://github.com/containers/podman.git synced 2025-05-17 15:18:43 +08:00
Code Issues Packages Projects Releases Wiki Activity

Fix a potential defer logic error around locking

Browse Source 
in several top-level API functions. These are the first line of
the function that contains them, which makes sense; we want to
capture any error returned by the function. However, making this
the first defer means that it is the last thing to run after the
function returns - meaning that the container's
`defer c.lock.Unlock()` has already fired, leading to a chance we
modify the container without holding its lock.

We could move the function around so it's no longer the first
defer, but then we'd have to call it twice (immediately after
`defer c.lock.Unlock()` if the container is not batched, and a
second time in a new `else` block right after the lock/sync call
to make sure we handle batched containers). Seems simpler to just
leave it like this.

[NO NEW TESTS NEEDED] Can't really test for DB corruption easily.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>
This commit is contained in:
Matthew Heon
2023-01-06 13:07:05 -05:00
parent 6f4eafe37c
commit 92cdad0315
1 changed files with 27 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
libpod/container_api.go
View File

@ -85,6 +85,15 @@ func (c *Container) Init(ctx context.Context, recursive bool) error {
func (c *Container) Start(ctx context.Context, recursive bool) (finalErr error) {
defer func() {
if finalErr != nil {
// Have to re-lock.
// As this is the first defer, it's the last thing to
// happen in the function - so `defer c.lock.Unlock()`
// below already fired.
if !c.batched {
c.lock.Lock()
defer c.lock.Unlock()
}
if err := saveContainerError(c, finalErr); err != nil {
logrus.Debug(err)
}
@ -125,6 +134,15 @@ func (c *Container) Update(res *spec.LinuxResources) error {
func (c *Container) StartAndAttach(ctx context.Context, streams *define.AttachStreams, keys string, resize <-chan resize.TerminalSize, recursive bool) (retChan <-chan error, finalErr error) {
defer func() {
if finalErr != nil {
// Have to re-lock.
// As this is the first defer, it's the last thing to
// happen in the function - so `defer c.lock.Unlock()`
// below already fired.
if !c.batched {
c.lock.Lock()
defer c.lock.Unlock()
}
if err := saveContainerError(c, finalErr); err != nil {
logrus.Debug(err)
}
@ -212,6 +230,15 @@ func (c *Container) Stop() error {
func (c *Container) StopWithTimeout(timeout uint) (finalErr error) {
defer func() {
if finalErr != nil {
// Have to re-lock.
// As this is the first defer, it's the last thing to
// happen in the function - so `defer c.lock.Unlock()`
// below already fired.
if !c.batched {
c.lock.Lock()
defer c.lock.Unlock()
}
if err := saveContainerError(c, finalErr); err != nil {
logrus.Debug(err)
}