search

rjeczalik / notify

File system event notification library on steroids.
MIT License
902 stars 128 forks source link

fsevents: Prevent nil deref #156

Closed imsodin closed 6 years ago

imsodin commented 6 years ago

Since using notify in Syncthing the following panic occurs randomly (see syncthing/syncthing#4854):

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

goroutine 19 [running, locked to thread]:
github.com/syncthing/syncthing/vendor/github.com/syncthing/notify.gostream(0x6b00c30, 0x4, 0x11, 0x6900ef0, 0x4df9000, 0x4df8000)
    /Users/jb/src/github.com/syncthing/syncthing/vendor/github.com/syncthing/notify/watcher_fsevents_cgo.go:107 +0x2d2
github.com/syncthing/syncthing/vendor/github.com/syncthing/notify._cgoexpwrap_4ce340c09c06_gostream(0x6b00c30, 0x4, 0x11, 0x6900ef0, 0x4df9000, 0x4df8000)
    _cgo_gotypes.go:399 +0x6b
github.com/syncthing/syncthing/vendor/github.com/syncthing/notify._Cfunc_CFRunLoopRun()
    _cgo_gotypes.go:214 +0x5f
github.com/syncthing/syncthing/vendor/github.com/syncthing/notify.init.1.func1()
    /Users/jb/src/github.com/syncthing/syncthing/vendor/github.com/syncthing/notify/watcher_fsevents_cgo.go:72 +0xce
created by github.com/syncthing/syncthing/vendor/github.com/syncthing/notify.init.1
    /Users/jb/src/github.com/syncthing/syncthing/vendor/github.com/syncthing/notify/watcher_fsevents_cgo.go:65 +0x5c
FAIL    github.com/syncthing/syncthing/lib/fs   0.764s

From a quick glance at the code it seems like this could be a race between unwatching and receiving an event from the C code. I believe this change should prevent a panic, but I do not know whether there is an underlying problem that should be addressed instead.

rjeczalik commented 6 years ago

Looks like indeed race between cgo handler and unwatch, I don't see off-hand those two can be synchronised. Thanks for the fix @imsodin!

PS I see tests on macos builder gets sigkilled - since it runs older 1.7 I thought it'd be worth a shot to update go version to the latest one, no problem with that?