c.RWLayer: check for nil before use #36242
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpuguy83
requested changes
Feb 8, 2018
daemon/changes.go
Outdated
Member
There was a problem hiding this comment.
We should probably check if the container is in the dead state first.
This would allow us to give a better error message.
Contributor
Author
There was a problem hiding this comment.
Just here or in mount/unmount as well?
kolyshkin
force-pushed
the
rwlayer-nil-deref
branch
3 times, most recently
from
10dcedf to
42ced1d
Compare
daemon/daemon.go
Outdated
Contributor
There was a problem hiding this comment.
+1 for checking for container.Dead here, unmount and in Windows, like in ContainerChanges
Contributor
Author
There was a problem hiding this comment.
Please see the updated version
Contributor
Author
There was a problem hiding this comment.
Well, adding a check for container.Dead is a change in workflow -- meaning we no longer mount/unmount containers in Dead state. I'm not quite sure if this is the right change....
Contributor
Author
There was a problem hiding this comment.
reverted the check for container.Dead in mount/umount/change, leaving only in getInspectData where a similar check was before

kolyshkin
force-pushed
the
rwlayer-nil-deref
branch
2 times, most recently
from
1a18157 to
9535192
Compare
Contributor
|
LGTM |
Member
|
There seems to be a deadlock in inspect. |
Member
|
Windows: |
Since commit e9b9e4a has landed, there is a chance that container.RWLayer is nil (due to some half-removed container). Let's check the pointer before use to avoid any potential nil pointer dereferences, resulting in a daemon crash. Note that even without the abovementioned commit, it's better to perform an extra check (even it's totally redundant) rather than to have a possibility of a daemon crash. In other words, better be safe than sorry. [v2: add a test case for daemon.getInspectData] [v3: add a check for container.Dead and a special error for the case] Fixes: e9b9e4a Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
rwlayer-nil-deref
branch
from
9535192 to
195893d
Compare
teqwve
added a commit
to RTBHOUSE/moby
that referenced
this pull request
Jan 7, 2025
Previously the RWLayer reference was cleared without holding the mutex. This could lead to goroutine panics in various places that use the container.RWLayer because nil checks introduced in moby#36242 where not sufficient as the reference could change after the check for nil and before the reference use. Signed-off-by: Tadeusz Dudkiewicz <[email protected]>
teqwve
added a commit
to RTBHOUSE/moby
that referenced
this pull request
Jan 8, 2025
Previously the RWLayer reference was cleared without holding the container lock. This could lead to goroutine panics in various places that use the container.RWLayer because nil checks introduced in moby#36242 where not sufficient as the reference could change right before the use. Fixes moby#49227 Signed-off-by: Tadeusz Dudkiewicz <[email protected]>
vvoland
pushed a commit
to vvoland/moby
that referenced
this pull request
Jan 8, 2025
Previously the RWLayer reference was cleared without holding the container lock. This could lead to goroutine panics in various places that use the container.RWLayer because nil checks introduced in moby#36242 where not sufficient as the reference could change right before the use. Fixes moby#49227 Signed-off-by: Tadeusz Dudkiewicz <[email protected]> (cherry picked from commit 97dc305) Signed-off-by: Paweł Gronowski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since commit e9b9e4a (#36160) has landed, there is a chance that
container.RWLayer is nil (due to some half-removed container). Let's
check the pointer before use to avoid any potential nil pointer
dereferences, resulting in a daemon crash.
Note that even without the abovementioned commit, it's better to perform
an extra check (even it's totally redundant) rather than to have a
possibility of a daemon crash. In other words, better be safe than
sorry.
Fixes: e9b9e4a
Signed-off-by: Kir Kolyshkin [email protected]
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)