Don't accidently remove XDG_RUNTIME_DIR when reseting storage #8750
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan 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 |
openshift-ci-robot
added
the
approved
| _, prevError := r.store.Shutdown(true) | ||
| if err := os.RemoveAll(r.store.GraphRoot()); err != nil { | ||
| graphRoot := r.store.GraphRoot() | ||
| if graphRoot == xdgRuntimeDir { |
There was a problem hiding this comment.
I'm not very familiar with Go, so is that a string equality, or does it handle minor differences between paths as they might have been defined, eg. with or without a trailing /.
There was a problem hiding this comment.
The filepath.Clean() will cleanup extra "/" and other cruft in the paths.
libpod/reset.go
Outdated
| @@ -77,18 +77,35 @@ func (r *Runtime) Reset(ctx context.Context) error { | |||
| } | |||
| } | |||
|
|
|||
| xdgRuntimeDir := os.Getenv("XDG_RUNTIME_DIR") | |||
There was a problem hiding this comment.
The documentation XDG_RUNTIME_DIR says:
If $XDG_RUNTIME_DIR is not set applications should fall back to a replacement directory with similar capabilities and print a warning message. Applications should use this directory for communication and synchronization purposes and should not place larger files in it, since it might reside in runtime memory and cannot necessarily be swapped out to disk.
So this should probably fallback to /run/user/<uid>.
| } | ||
| if err := os.RemoveAll(r.store.RunRoot()); err != nil { | ||
| runRoot := r.store.RunRoot() | ||
| if runRoot == xdgRuntimeDir { |
There was a problem hiding this comment.
It might be worth checking whether the "run root" is correctly prefixed, if it is a child of xdgRuntimeDir.
There was a problem hiding this comment.
It matters if you can only do string comparisons. The new default configuration is $XDG_RUNTIME_DIR/overlays, it used to be $XDG_RUNTIME_DIR. You still don't want to nuke $XDG_RUNTIME_DIR/.
There was a problem hiding this comment.
Right the case above will only remove $XDG_RUNTIME_DIR/overlays which is what we want. It really does not mapper if XDG_RUNTIME_DIR/overlays or /random/path/overlays
|
I'm slightly concerned about using string equality rather than path equality here, but looks good otherwise. |
libpod/reset.go
Outdated
| if prevError != nil { | ||
| logrus.Error(prevError) | ||
| } | ||
| prevError = err | ||
| prevError = errors.Errorf("failed to remove runtime tmpdir %s, since it is the same XDG_RUNTIME_DIR", tempDir) |
There was a problem hiding this comment.
nit
| prevError = errors.Errorf("failed to remove runtime tmpdir %s, since it is the same XDG_RUNTIME_DIR", tempDir) | |
| prevError = errors.Errorf("failed to remove runtime tmpdir %s, since it is the same as XDG_RUNTIME_DIR", tempDir) |
libpod/reset.go
Outdated
| if prevError != nil { | ||
| logrus.Error(prevError) | ||
| } | ||
| prevError = err | ||
| prevError = errors.Errorf("failed to remove runtime root dir %s, since it is the same XDG_RUNTIME_DIR", runRoot) |
There was a problem hiding this comment.
| prevError = errors.Errorf("failed to remove runtime root dir %s, since it is the same XDG_RUNTIME_DIR", runRoot) | |
| prevError = errors.Errorf("failed to remove runtime root dir %s, since it is the same as XDG_RUNTIME_DIR", runRoot) |
|
Couple small nits, LGTM once cleared. |
There was a problem hiding this comment.
LGTM
I like the filepath clean bit, will have to try and remember that for the future. Good call @hadess
In certain cases XDG_RUNTIME_DIR was deleted by accident based on settings in the storage.conf. This patch verifies that when doing a storage reset, we don't accidently remove XDG_RUNTIME_DIR. Fixes: containers#8680 Signed-off-by: Daniel J Walsh <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
In certain cases XDG_RUNTIME_DIR was deleted by accident based on
settings in the storage.conf. This patch verifies that when doing
a storage reset, we don't accidently remove XDG_RUNTIME_DIR.
Fixes: #8680
Signed-off-by: Daniel J Walsh [email protected]