fifo.Close(): prevent possible panic if fifo is nil by thaJeztah · Pull Request #32 · containerd/fifo

thaJeztah · 2021-01-29T13:56:05Z

I'm not sure if this is the right approach, and synchronisation should probably be added elsewhere to fix the underlying issue.

Trying to prevent a panic that was seen on container restore in th docker daemon:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x5586c892a7a4]

goroutine 420 [running]:
github.com/docker/docker/vendor/github.com/containerd/fifo.(*fifo).Close(0x0, 0x0, 0x0)
        /go/src/github.com/docker/docker/vendor/github.com/containerd/fifo/fifo.go:208 +0x44
github.com/docker/docker/vendor/github.com/containerd/containerd/cio.(*cio).Close(0xc000d06f60, 0x5586cb5654d0, 0xc000d8e9e8)
        /go/src/github.com/docker/docker/vendor/github.com/containerd/containerd/cio/io.go:203 +0x90
github.com/docker/docker/libcontainerd/remote.(*client).Restore.func1(0xc0008bf820, 0xc0008a2040)
        /go/src/github.com/docker/docker/libcontainerd/remote/client.go:86 +0x5a
github.com/docker/docker/libcontainerd/remote.(*client).Restore(0xc00098e5b0, 0x5586cb61c7c0, 0xc000052088, 0xc0011b6500, 0x40, 0xc0008bf810, 0x5586cb05cf00, 0xffffffffffffffff, 0x0, 0x0, ...)
        /go/src/github.com/docker/docker/libcontainerd/remote/client.go:107 +0x923
github.com/docker/docker/daemon.(*Daemon).restore.func3(0xc00079d9e0, 0xc000a38230, 0xc00000c1e0, 0xc00079d9a8, 0xc000d84f00, 0xc000d84ed0, 0xc000d84ea0, 0xc00128a280)
        /go/src/github.com/docker/docker/daemon/daemon.go:351 +0x48a
created by github.com/docker/docker/daemon.(*Daemon).restore
        /go/src/github.com/docker/docker/daemon/daemon.go:319 +0x4b3

If the fifo is nil, there's nothing to be done in Close(), so returning early in that situation.

thaJeztah · 2021-01-29T13:56:21Z

@cpuguy83 @AkihiroSuda ptal

thaJeztah · 2021-01-29T13:57:34Z

fifo.go

 func (f *fifo) Close() (retErr error) {
 	for {
+		if f == nil {
+			return


This method can also be used before open(2) has returned and fifo was never opened.

Also not sure if we should

return an error in this case (currently it would use retErr, if set)

return nil (which would unset retErr if it was set before)

I think the caller should check for nil.

Could it become nil while we're in this for{} loop?

I should hope not... if so then that would be the bug that should be fixed.

Hmm... so I suspect that something like that actually happens (somehow); following that trace, the panic followed from this code; https://github.com/containerd/containerd/blob/269548fa27e0089a8b8278fc4fc781d7f65a939b/cio/io.go#L199-L206

func (c *cio) Close() error { var lastErr error for _, closer := range c.closers { if closer == nil { continue } if err := closer.Close(); err != nil { lastErr = err } } return lastErr }

So there's already a nil check there. So only other explanation would be there there's some concurrency happening, in which case do we need locking?

could have been explain by the infamous interface is nil problem, either we make sure that we dont pass nil io.Closer to cio, or do a type check ( yuck), or we check inside fifo.Close for nil.

Ah, sorry, missed your comment here, @dqminh - do you mean we should use a different fix, or is the current "fix" ok?

i think the best case is that we need to make sure the caller doesn't pass a nil interface as that can be nasty too if we call anything other than Close ( which is fixed in this way ). The current check we are doing here is not the end of the world, but ideally i don't think we need it at this place.

thaJeztah · 2021-01-29T13:59:57Z

OK, looks like this is failing;

=== RUN   TestRawReadWrite
    raw_test.go:54: 
        	Error Trace:	raw_test.go:54
        	Error:      	An error is expected but got nil.
        	Test:       	TestRawReadWrite

I'm not sure if this is the right approach, and synchronisation should probably be added elsewhere to fix the underlying issue. Trying to prevent a panic that was seen on container restore in th docker daemon: panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x5586c892a7a4] goroutine 420 [running]: github.com/docker/docker/vendor/github.com/containerd/fifo.(*fifo).Close(0x0, 0x0, 0x0) /go/src/github.com/docker/docker/vendor/github.com/containerd/fifo/fifo.go:208 +0x44 github.com/docker/docker/vendor/github.com/containerd/containerd/cio.(*cio).Close(0xc000d06f60, 0x5586cb5654d0, 0xc000d8e9e8) /go/src/github.com/docker/docker/vendor/github.com/containerd/containerd/cio/io.go:203 +0x90 github.com/docker/docker/libcontainerd/remote.(*client).Restore.func1(0xc0008bf820, 0xc0008a2040) /go/src/github.com/docker/docker/libcontainerd/remote/client.go:86 +0x5a github.com/docker/docker/libcontainerd/remote.(*client).Restore(0xc00098e5b0, 0x5586cb61c7c0, 0xc000052088, 0xc0011b6500, 0x40, 0xc0008bf810, 0x5586cb05cf00, 0xffffffffffffffff, 0x0, 0x0, ...) /go/src/github.com/docker/docker/libcontainerd/remote/client.go:107 +0x923 github.com/docker/docker/daemon.(*Daemon).restore.func3(0xc00079d9e0, 0xc000a38230, 0xc00000c1e0, 0xc00079d9a8, 0xc000d84f00, 0xc000d84ed0, 0xc000d84ea0, 0xc00128a280) /go/src/github.com/docker/docker/daemon/daemon.go:351 +0x48a created by github.com/docker/docker/daemon.(*Daemon).restore /go/src/github.com/docker/docker/daemon/daemon.go:319 +0x4b3 If the fifo is nil, there's nothing to be done in Close(), so returning early in that situation. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2021-01-29T14:09:47Z

Hmm... looks like there's some flakiness; re-pushed, and now it passes; could it be related to Go 1.14/ Go 1.15?

thaJeztah · 2021-01-29T14:15:41Z

Ran Tibor's script from https://gist.github.com/tiborvass/eb0a4054679a43aaca22690a7c4452ed on this repository, in case it's useful;

Details

./check-syscalls.sh
Acct SYS_ACCT
AddKey SYS_ADD_KEY
Adjtimex SYS_ADJTIMEX
Chdir SYS_CHDIR
Chroot SYS_CHROOT
ClockGetres SYS_CLOCK_GETRES
ClockGettime SYS_CLOCK_GETTIME
ClockNanosleep SYS_CLOCK_NANOSLEEP
Close SYS_CLOSE
CopyFileRange SYS_COPY_FILE_RANGE
DeleteModule SYS_DELETE_MODULE
Dup SYS_DUP
Dup2 SYS_DUP2
Dup3 SYS_DUP3
EpollWait SYS_EPOLL_WAIT
Eventfd SYS_EVENTFD2
Exit SYS_EXIT_GROUP
Faccessat2 SYS_FACCESSAT2
Fadvise SYS_FADVISE64
Fallocate SYS_FALLOCATE
FanotifyInit SYS_FANOTIFY_INIT
Fchdir SYS_FCHDIR
Fchmod SYS_FCHMOD
Fchown SYS_FCHOWN
Fchownat SYS_FCHOWNAT
Fdatasync SYS_FDATASYNC
Fgetxattr SYS_FGETXATTR
FinitModule SYS_FINIT_MODULE
Flistxattr SYS_FLISTXATTR
Flock SYS_FLOCK
Fremovexattr SYS_FREMOVEXATTR
Fsetxattr SYS_FSETXATTR
Fstat SYS_FSTAT
Fstatat SYS_NEWFSTATAT
Fstatfs SYS_FSTATFS
Fsync SYS_FSYNC
Ftruncate SYS_FTRUNCATE
Getcwd SYS_GETCWD
Getdents SYS_GETDENTS64
Getpriority SYS_GETPRIORITY
Getrandom SYS_GETRANDOM
Getxattr SYS_GETXATTR
InitModule SYS_INIT_MODULE
InotifyAddWatch SYS_INOTIFY_ADD_WATCH
Ioperm SYS_IOPERM
Iopl SYS_IOPL
KeyctlBuffer SYS_KEYCTL
KeyctlInt SYS_KEYCTL
Klogctl SYS_SYSLOG
Lchown SYS_LCHOWN
Lgetxattr SYS_LGETXATTR
Linkat SYS_LINKAT
Listen SYS_LISTEN
Listxattr SYS_LISTXATTR
Llistxattr SYS_LLISTXATTR
Lremovexattr SYS_LREMOVEXATTR
Lsetxattr SYS_LSETXATTR
Madvise SYS_MADVISE
MemfdCreate SYS_MEMFD_CREATE
Mkdirat SYS_MKDIRAT
Mknodat SYS_MKNODAT
Mlock SYS_MLOCK
Mlockall SYS_MLOCKALL
Mprotect SYS_MPROTECT
Msync SYS_MSYNC
Munlock SYS_MUNLOCK
Munlockall SYS_MUNLOCKALL
Nanosleep SYS_NANOSLEEP
Pause SYS_PAUSE
PerfEventOpen SYS_PERF_EVENT_OPEN
PivotRoot SYS_PIVOT_ROOT
Prctl SYS_PRCTL
Pread SYS_PREAD64
ProcessVMReadv SYS_PROCESS_VM_READV
ProcessVMWritev SYS_PROCESS_VM_WRITEV
Pselect SYS_PSELECT6
Pwrite SYS_PWRITE64
Readlinkat SYS_READLINKAT
Removexattr SYS_REMOVEXATTR
Renameat SYS_RENAMEAT
Renameat2 SYS_RENAMEAT2
RequestKey SYS_REQUEST_KEY
Seek SYS_LSEEK
Select SYS_SELECT
Setdomainname SYS_SETDOMAINNAME
Setfsgid SYS_SETFSGID
Setfsuid SYS_SETFSUID
Sethostname SYS_SETHOSTNAME
Setns SYS_SETNS
Setpriority SYS_SETPRIORITY
Setxattr SYS_SETXATTR
Shutdown SYS_SHUTDOWN
Splice SYS_SPLICE
Statfs SYS_STATFS
Statx SYS_STATX
Symlinkat SYS_SYMLINKAT
Sync SYS_SYNC
SyncFileRange SYS_SYNC_FILE_RANGE
Syncfs SYS_SYNCFS
Tee SYS_TEE
Truncate SYS_TRUNCATE
Unlinkat SYS_UNLINKAT
Unmount SYS_UMOUNT2
Unshare SYS_UNSHARE
Ustat SYS_USTAT
Utime SYS_UTIME

thaJeztah · 2021-02-18T21:09:35Z

@cpuguy83 @AkihiroSuda ptal

thaJeztah · 2021-03-23T09:37:16Z

@AkihiroSuda any thoughts on the discussion above? #32 (comment) (i.e.; would fixes be needed elsewhere (as well?)

fuweid

LGTM

kzys

Looks good to me since it would fix a production issue. I'm in "the caller should check that" camp though :)

Rid · 2022-01-27T10:31:24Z

@thaJeztah Did this make it into a release? I'm still seeing these issues occasionally on v20.10.12, with containerd 1.4.12 7b11cfaabd73bb80907dd23182b9347b4245eb5d.

I can't seem to find a way to resolve the issue without restarting the entire server, which is very costly.

Jan 27 11:05:55  cli[2633740]: time="2022-01-27T11:05:55.879409823+01:00" level=info msg="Starting up"
Jan 27 11:05:55  cli[2633740]: time="2022-01-27T11:05:55.879804137+01:00" level=info msg="User namespaces: ID ranges will be mapped to subuid/subgid ranges of: 11513"
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.373259438+01:00" level=info msg="User namespaces: ID ranges will be mapped to subuid/subgid ranges of: 11513"
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.375457580+01:00" level=info msg="parsed scheme: \"unix\"" module=grpc
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.375505639+01:00" level=info msg="scheme \"unix\" not registered, fallback to default scheme" module=grpc
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.375552415+01:00" level=info msg="ccResolverWrapper: sending update to cc: {[{unix:///run/containerd/containerd.sock  <nil> 0 <nil>}] <nil> <nil>}" module=grpc
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.375578423+01:00" level=info msg="ClientConn switching balancer to \"pick_first\"" module=grpc
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.379642047+01:00" level=info msg="parsed scheme: \"unix\"" module=grpc
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.379733149+01:00" level=info msg="scheme \"unix\" not registered, fallback to default scheme" module=grpc
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.379806034+01:00" level=info msg="ccResolverWrapper: sending update to cc: {[{unix:///run/containerd/containerd.sock  <nil> 0 <nil>}] <nil> <nil>}" module=grpc
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.379840347+01:00" level=info msg="ClientConn switching balancer to \"pick_first\"" module=grpc
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.760278807+01:00" level=warning msg="Your kernel does not support CPU realtime scheduler"
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.760346178+01:00" level=warning msg="Your kernel does not support cgroup blkio weight"
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.760361162+01:00" level=warning msg="Your kernel does not support cgroup blkio weight_device"
Jan 27 11:05:56  cli[2633740]: time="2022-01-27T11:05:56.760735829+01:00" level=info msg="Loading containers: start."
Jan 27 11:05:56  cli[2633740]: panic: runtime error: invalid memory address or nil pointer dereference
Jan 27 11:05:56  cli[2633740]: [signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x563e12aa9824]
Jan 27 11:05:56  cli[2633740]: goroutine 158 [running]:
Jan 27 11:05:56  cli[2633740]: github.com/docker/docker/vendor/github.com/containerd/fifo.(*fifo).Close(0x0, 0x0, 0x0)
Jan 27 11:05:56  cli[2633740]:         /go/src/github.com/docker/docker/vendor/github.com/containerd/fifo/fifo.go:208 +0x44
Jan 27 11:05:56  cli[2633740]: github.com/docker/docker/vendor/github.com/containerd/containerd/cio.(*cio).Close(0xc000e71230, 0xc000601528, 0xc0012bca58)
Jan 27 11:05:56  cli[2633740]:         /go/src/github.com/docker/docker/vendor/github.com/containerd/containerd/cio/io.go:203 +0x90
Jan 27 11:05:56  cli[2633740]: github.com/docker/docker/libcontainerd/remote.(*client).Restore.func1(0xc00029d230, 0xc00072e148)
Jan 27 11:05:56  cli[2633740]:         /go/src/github.com/docker/docker/libcontainerd/remote/client.go:86 +0x5a
Jan 27 11:05:56  cli[2633740]: github.com/docker/docker/libcontainerd/remote.(*client).Restore(0xc000dc8380, 0x563e14df4128, 0xc000052028, 0xc0002e8040, 0x40, 0xc00029d220, 0xc001302d00, 0xffffffffffffffff, 0x0, 0x0, ...)
Jan 27 11:05:56  cli[2633740]:         /go/src/github.com/docker/docker/libcontainerd/remote/client.go:107 +0xa07
Jan 27 11:05:56  cli[2633740]: github.com/docker/docker/daemon.(*Daemon).restore.func3(0xc000164300, 0xc0007281e0, 0xc00000c1e0, 0xc0001642b8, 0xc000b71080, 0xc000b71050, 0xc000b71020, 0xc00019a280)
Jan 27 11:05:56  cli[2633740]:         /go/src/github.com/docker/docker/daemon/daemon.go:351 +0x46d
Jan 27 11:05:56  cli[2633740]: created by github.com/docker/docker/daemon.(*Daemon).restore
Jan 27 11:05:56  cli[2633740]:         /go/src/github.com/docker/docker/daemon/daemon.go:319 +0x4cf

thaJeztah · 2022-01-27T10:38:06Z

Not this was not in a release yet; v1.0.0...39bc37d

I'm not sure if this is the right approach, and synchronisation should probably be added elsewhere to fix the underlying issue. Trying to prevent a panic that was seen on container restore in th docker daemon: panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0x5586c892a7a4] goroutine 420 [running]: github.com/docker/docker/vendor/github.com/containerd/fifo.(*fifo).Close(0x0, 0x0, 0x0) /go/src/github.com/docker/docker/vendor/github.com/containerd/fifo/fifo.go:208 +0x44 github.com/docker/docker/vendor/github.com/containerd/containerd/cio.(*cio).Close(0xc000d06f60, 0x5586cb5654d0, 0xc000d8e9e8) /go/src/github.com/docker/docker/vendor/github.com/containerd/containerd/cio/io.go:203 +0x90 github.com/docker/docker/libcontainerd/remote.(*client).Restore.func1(0xc0008bf820, 0xc0008a2040) /go/src/github.com/docker/docker/libcontainerd/remote/client.go:86 +0x5a github.com/docker/docker/libcontainerd/remote.(*client).Restore(0xc00098e5b0, 0x5586cb61c7c0, 0xc000052088, 0xc0011b6500, 0x40, 0xc0008bf810, 0x5586cb05cf00, 0xffffffffffffffff, 0x0, 0x0, ...) /go/src/github.com/docker/docker/libcontainerd/remote/client.go:107 +0x923 github.com/docker/docker/daemon.(*Daemon).restore.func3(0xc00079d9e0, 0xc000a38230, 0xc00000c1e0, 0xc00079d9a8, 0xc000d84f00, 0xc000d84ed0, 0xc000d84ea0, 0xc00128a280) /go/src/github.com/docker/docker/daemon/daemon.go:351 +0x48a created by github.com/docker/docker/daemon.(*Daemon).restore /go/src/github.com/docker/docker/daemon/daemon.go:319 +0x4b3 If the fifo is nil, there's nothing to be done in Close(), so returning early in that situation. Backport: <containerd/fifo#32> SUSE-Bugs: bsc#1200022 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah commented Jan 29, 2021

View reviewed changes

thaJeztah force-pushed the prevent_npe branch from bb32f37 to 80cf87d Compare January 29, 2021 14:08

AkihiroSuda approved these changes Mar 23, 2021

View reviewed changes

AkihiroSuda requested review from cpuguy83 and fuweid March 23, 2021 09:34

fuweid approved these changes Mar 23, 2021

View reviewed changes

kzys approved these changes Aug 12, 2021

View reviewed changes

dmcgowan approved these changes Sep 15, 2021

View reviewed changes

dmcgowan merged commit 60d22e8 into containerd:main Sep 15, 2021

thaJeztah deleted the prevent_npe branch September 15, 2021 21:38

thaJeztah mentioned this pull request Jan 27, 2022

Add ReaderFrom/WriterTo for Linux (splice) #29

Open

corhere mentioned this pull request Nov 8, 2022

Return nil when openFifo returns nil #47

Merged

corhere mentioned this pull request Feb 15, 2023

dockerd crashes on restart with live-restart enabled and specific syslog setup moby/moby#45002

Closed

Conversation

thaJeztah commented Jan 29, 2021

Uh oh!

thaJeztah commented Jan 29, 2021

Uh oh!

thaJeztah Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

cpuguy83 Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

dqminh Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

dqminh Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 29, 2021

Uh oh!

thaJeztah commented Jan 29, 2021

Uh oh!

thaJeztah commented Jan 29, 2021

Uh oh!

thaJeztah commented Feb 18, 2021

Uh oh!

thaJeztah commented Mar 23, 2021

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

kzys left a comment

Choose a reason for hiding this comment

Uh oh!

Rid commented Jan 27, 2022

Uh oh!

thaJeztah commented Jan 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

thaJeztah Jan 29, 2021 •

edited

Loading