Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

panic: nil pointer dereference in (*indexHandlerRegistry).startLocked #9269

Closed
syncthing-sentry bot opened this issue Dec 7, 2023 · 3 comments
Closed

panic: nil pointer dereference in (*indexHandlerRegistry).startLocked #9269

syncthing-sentry bot opened this issue Dec 7, 2023 · 3 comments
Labels
bug A problem with current functionality, as opposed to missing functionality (enhancement)
Milestone
v1.27.1

Comments

@syncthing-sentry
Copy link

syncthing-sentry bot commented Dec 7, 2023 •
edited by calmh 

2023/12/07 21:27:41 INFO: syncthing v1.27.0 "Gold Grasshopper" (go1.20.11 linux-amd64) root@buildkitsandbox 2023-12-05 07:12:09 UTC [noupgrade]
Panic at 2023-12-07T21:35:02+01:00
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x60 pc=0xd61b6e]

goroutine 171032 [running]:
github.com/syncthing/syncthing/lib/model.(*indexHandlerRegistry).startLocked(_, {{0xc00512dbc0, 0xb}, {0xc00512dbd0, 0xe}, 0x0, {0xc00116e9e0, 0x20}, 0x0, {0xc000e1bba0, ...}, ...}, ...)
	github.com/syncthing/syncthing/lib/model/indexhandler.go:419 +0x16e
github.com/syncthing/syncthing/lib/model.(*indexHandlerRegistry).AddIndexInfo(0xc0015e9400, {0xc0009d0ae0, 0xb}, 0xc00175a500)
	github.com/syncthing/syncthing/lib/model/indexhandler.go:439 +0x4b5
github.com/syncthing/syncthing/lib/model.(*model).ccHandleFolders(_, {_, _, _}, {{0x81, 0xc1, 0x8d, 0x7b, 0xd9, 0x97, ...}, ...}, ...)
	github.com/syncthing/syncthing/lib/model/model.go:1431 +0xca6
github.com/syncthing/syncthing/lib/model.(*model).ClusterConfig(0xc000002000, {0x13c2688, 0xc000fe6170}, {{0xc0011ac000?, 0xc0000d0088?, 0xc001429d78?}, 0xcf?})
	github.com/syncthing/syncthing/lib/model/model.go:1296 +0x8e8
github.com/syncthing/syncthing/lib/protocol.(*connectionWrappingModel).ClusterConfig(0xc001429d78?, {{0xc0011ac000?, 0x10?, 0xe96c40?}, 0x1?})
	github.com/syncthing/syncthing/lib/protocol/protocol.go:1143 +0x43
github.com/syncthing/syncthing/lib/protocol.encryptedModel.ClusterConfig(...)
	github.com/syncthing/syncthing/lib/protocol/encryption.go:155
github.com/syncthing/syncthing/lib/protocol.(*rawConnection).dispatcherLoop(0xc000d9b9e0)
	github.com/syncthing/syncthing/lib/protocol/protocol.go:496 +0x3b7
github.com/syncthing/syncthing/lib/protocol.(*rawConnection).Start.func2()
	github.com/syncthing/syncthing/lib/protocol/protocol.go:307 +0x25
created by github.com/syncthing/syncthing/lib/protocol.(*rawConnection).Start
	github.com/syncthing/syncthing/lib/protocol/protocol.go:306 +0x116

runner is nil here:

syncthing/lib/model/indexhandler.go

Line 419 in 75310b5

runner.SchedulePull()

//@calmh
@calmh
Copy link
Member

calmh commented Dec 7, 2023

Same as #9234 which we thought fixed.

@calmh
Copy link
Member

calmh commented Dec 7, 2023

Obvious culprits are 5118538 and c6334e6 where the latter added the incorrect locking subsequently fixed, but which was apparently not the root cause...

calmh added a commit to calmh/syncthing that referenced this issue Dec 7, 2023
@calmh
lib/model: Correct locking when removing folder runner (ref syncthing…
44a9478 
…#9269)

A read lock is insufficient because the remove of the map entry happens
in the RemoveAndWait call, where previously it didn't.

However! I'm not sure this actually fixes the problem, because we then
release the lock and have a nil/unset runner for a while, which
previously would happen later under fmut again...
@imsodin
Copy link
Member

imsodin commented Dec 7, 2023

Ah crap, I reopened that issue - I'll close it again then :)

@imsodin imsodin mentioned this issue Dec 7, 2023
Closed
imsodin added a commit to imsodin/syncthing that referenced this issue Dec 7, 2023
@imsodin
lib/model: Remove runner during folder cleanup (fixes syncthing#9269)
87b724d 
Before introducing the service map and using it for folder runners, the
entries in folderCfgs and folderRunners for the same key/folder were
removed under a single lock. Stopping the folder happens separately
before that with just the read lock. Now with the service map stopping the
folder and removing it from the map is a single operation. And that
still happens with just a read-lock. However even with a full lock it’s still
problematic: After the folder stopped, the runner isn’t present anymore while
the folder-config still is and sais the folder isn't paused.

The index handler in turn looks at the folder config that is not paused,
thus assumes the runner has to be present -> nil deref on the runner.

A better solution would like be to push most of these fmut maps into the
folder - they anyway are needed in there. Then there's just a single
map/source of info that's necessarily consistent.
imsodin added a commit to imsodin/syncthing that referenced this issue Dec 7, 2023
@imsodin
lib/model: Remove runner during folder cleanup (fixes syncthing#9269)
bb5cd15 
Before introducing the service map and using it for folder runners, the
entries in folderCfgs and folderRunners for the same key/folder were
removed under a single lock. Stopping the folder happens separately
before that with just the read lock. Now with the service map stopping the
folder and removing it from the map is a single operation. And that
still happens with just a read-lock. However even with a full lock it’s still
problematic: After the folder stopped, the runner isn’t present anymore while
the folder-config still is and sais the folder isn't paused.

The index handler in turn looks at the folder config that is not paused,
thus assumes the runner has to be present -> nil deref on the runner.

A better solution would like be to push most of these fmut maps into the
folder - they anyway are needed in there. Then there's just a single
map/source of info that's necessarily consistent.
@calmh calmh closed this as completed in a28de73 Dec 8, 2023
@calmh calmh added this to the v1.27.1 milestone Dec 8, 2023
@calmh calmh added the bug A problem with current functionality, as opposed to missing functionality (enhancement) label Dec 8, 2023
@acolomb acolomb mentioned this issue Dec 10, 2023
Closed
calmh added a commit to cjc7373/syncthing that referenced this issue Dec 11, 2023
@calmh
Merge branch 'main' into cli-framework
e44b2e2 
* main: (69 commits)
  lib/nat: Fix test build failure (ref syncthing#9010)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010)
  gui, man, authors: Update docs, translations, and contributors
  gui: Show folder/device status on small screens (syncthing#8643)
  lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
  build: Update dependencies (syncthing#9265)
  build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264)
  lib/fs: Reduce memory usage in xattrs handling (syncthing#9251)
  lib/model: Improve LastSeen handling (syncthing#9256)
  lib/scanner: Record inode change time for directories and symlinks (syncthing#9250)
  lib/api: Improve ignore loading error handling (fixes syncthing#9253) (syncthing#9254)
  gui, man, authors: Update docs, translations, and contributors
  lib/fs: Better equality comparison in mtimefs
  cmd/stcrashreceiver: Add metrics for diskstore inventory
  cmd/stcrashreceiver: Minor cleanup, stricter file permissions
  cmd/stcrashreceiver: Add metrics for incoming reports
  ...
calmh added a commit to calmh/syncthing that referenced this issue Dec 16, 2023
@calmh
Merge branch 'main' into fs/add-encoder
64aa109 
* main: (89 commits)
  build: Update quic-go (fixes syncthing#9287)
  lib/model: Only handle relevant folder summaries (kqueue) (fixes syncthing#9183) (syncthing#9288)
  lib/model: Use a single lock (phase two: cleanup) (syncthing#9276)
  build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279)
  lib/model: Use a single lock (syncthing#9275)
  cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281)
  cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166)
  lib/nat: Fix test build failure (ref syncthing#9010)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010)
  gui, man, authors: Update docs, translations, and contributors
  gui: Show folder/device status on small screens (syncthing#8643)
  lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
  build: Update dependencies (syncthing#9265)
  build: Revert specifics for Go 1.21.4, build using Go 1.21.5 (syncthing#9264)
  lib/fs: Reduce memory usage in xattrs handling (syncthing#9251)
  lib/model: Improve LastSeen handling (syncthing#9256)
  ...
calmh added a commit to calmh/syncthing that referenced this issue Jan 14, 2024
@calmh
Merge branch 'main' into integrationtests
f701994 
* main:
  lib/model: Use a single lock (phase two: cleanup) (syncthing#9276)
  build(deps): bump actions/setup-go from 4 to 5 (syncthing#9279)
  lib/model: Use a single lock (syncthing#9275)
  cmd/syncthing: Better cli stdin handling (ref syncthing#9166) (syncthing#9281)
  cmd/syncthing: Mostly replace urfave/cli command line parser with alecthomas/kong (syncthing#9166)
  lib/nat: Fix test build failure (ref syncthing#9010)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Add pmut locking for DeviceStatistics (fixes syncthing#9274)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/model: Remove spurious "replacing service" failure event (ref syncthing#9271)
  lib/nat, lib/upnp: IPv6 UPnP support (syncthing#9010)
  gui, man, authors: Update docs, translations, and contributors
  gui: Show folder/device status on small screens (syncthing#8643)
  lib/model: Remove runner during folder cleanup (fixes syncthing#9269) (syncthing#9271)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug A problem with current functionality, as opposed to missing functionality (enhancement)
Projects
None yet
Milestone
v1.27.1
Development

No branches or pull requests
2 participants
@calmh @imsodin