cio: prevent NPE when closing, and fix pipes potentially not being closed on Windows #4980
cio: prevent NPE when closing, and fix pipes potentially not being closed on Windows #4980estesp merged 4 commits intocontainerd:masterfrom
Conversation
|
Skipping CI for Draft Pull Request. |
|
Build succeeded.
|
|
@cpuguy83 @AkihiroSuda ptal 🤗 |
There was a problem hiding this comment.
Would it be simpler to not use a named return value for cio (_ *cio, retErr error), then just return nil here instead? Seems like we would prefer to return nil when we have an error, and I think we're only returning this due to the named return value.
There was a problem hiding this comment.
That's roughly what's in the 3rd commit, but does mean that we have to do the closing by iterating over the set slice; I thought we might as well use cio.Close() to do that for us (and skip the intermediate set slice)
There was a problem hiding this comment.
Oh! I think I understand what you mean; so make cios a local variable (but still use it in the defer); is that what you meant?
There was a problem hiding this comment.
Yeah, something like this:
func copyIO(fifos *FIFOSet, ioset *Streams) (_ *cio, retErr error) { // cios changed to _
cios := &cio{config: fifos.Config} // = changed to :=
[...]
return nil, errors.Wrapf(err, "failed to create stdin pipe %s", fifos.Stdin)
There was a problem hiding this comment.
That also lets you simplify the defer to just:
defer func() {
if retErr != nil {
_ = cios.Close()
}
}()
There was a problem hiding this comment.
Ah, yes, I like that.
technically, we shouldn't have to set it to nil as no consumer should handle the value on error, but I wanted to keep the behavior as it was, just in case.
I'll have a look at updating the PR; thanks!
Signed-off-by: Sebastiaan van Stijn <[email protected]>
This change is mostly defensive; when checking for the returned error, it's easy to make a mistake, and check for a "local" error, not the actual returned error. This patch changes the function to use a named return variable, which is checked in the defer. Signed-off-by: Sebastiaan van Stijn <[email protected]>
The defer functions were checking the local variable, and would therefore
not be executed, as the function returned if an error occurred.
Perhaps best illustrated when renaming the local variables;
if fifos.Stdin != "" {
l, err1 := winio.ListenPipe(fifos.Stdin, nil)
if err1 != nil {
return nil, errors.Wrapf(err1, "failed to create stdin pipe %s", fifos.Stdin)
}
defer func(l net.Listener) {
if err1 != nil {
l.Close()
}
}(l)
// ...
}
if fifos.Stdout != "" {
l, err2 := winio.ListenPipe(fifos.Stdout, nil)
if err2 != nil {
return nil, errors.Wrapf(err2, "failed to create stdout pipe %s", fifos.Stdout)
}
defer func(l net.Listener) {
if err2 != nil {
l.Close()
}
}(l)
// ....
}
This patch changes the function to use a named return variable, and to use
a single `defer()` that closes all pipes.
Signed-off-by: Sebastiaan van Stijn <[email protected]>
Use the existing `.Close()` method instead of implementing the same logic in this function. The defer sets `cios` to `nil` if an error occurred to preserve the existing behavior. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
prevent_cio_npe
branch
from
e384777 to
7a468a3
Compare
|
Build succeeded.
|
|
@kevpar updated; ptal 👍 |
|
@estesp @AkihiroSuda ptal 🤗 |
4 participants
1. cio: FIFOSet.Close() check if FIFOSet is nill to prevent NPE
This relates to docker/for-linux#1186. Perhaps this needs synchronisation elsewhere, but looks like this could potentially panic here as well. Seems not harmful to check for
nil.2. cio: openFifos() use named return variables to use in defer()
This change is mostly defensive; when checking for the returned error, it's easy to make a mistake, and check for a "local" error, not the actual returned error.
This patch changes the function to use a named return variable, which is checked in the defer.
3. cio.copyIO: fix pipes potentially not being closed (Windows)
The defer functions were checking the local variable, and would therefore not be executed, as the function returned if an error occurred.
Perhaps best illustrated when renaming the local variables;
This patch changes the function to use a named return variable, and to use a single
defer()that closes all pipes.4. cio.copyIO: refactor to use cio.Close() (windows)
A follow-up to the patch above; Use the existing
.Close()method instead of implementing the samelogic in this function.
The defer sets
ciostonilif an error occurred to preserve the existing behavior.