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

Nil deref in index handler #9234

Closed
imsodin opened this issue Nov 19, 2023 · 2 comments
Closed

Nil deref in index handler #9234

imsodin opened this issue Nov 19, 2023 · 2 comments
Labels
bug A problem with current functionality, as opposed to missing functionality (enhancement)
Milestone
v1.27.0

Comments

@imsodin
Copy link
Member

imsodin commented Nov 19, 2023

Panic were reported in android a few times and then here an actual/ful panic trace was referenced:
https://forum.syncthing.net/t/syncthing-1-26-crashing-on-android-in-syncthing-fork/21115/2

11-19 11:32:51.661 11506 21478 W SyncthingNativeCode: panic: runtime error: invalid memory address or nil pointer dereference
11-19 11:32:51.661 11506 21478 W SyncthingNativeCode: [signal SIGSEGV: segmentation violation code=0x1 addr=0x60 pc=0x637d5ee060]
11-19 11:32:51.661 11506 21478 W SyncthingNativeCode:
11-19 11:32:51.661 11506 21478 W SyncthingNativeCode: goroutine 325 [running]:
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/model.(*indexHandlerRegistry).startLocked(, {{0x4000363e44, 0xc}, {0x4000363e70, 0xc}, 0x0, {0x400011af78, 0x18}, 0x1, {0x40000c2b40, ...}, ...}, ...)
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/model/indexhandler.go:419 +0x100
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/model.(*indexHandlerRegistry).AddIndexInfo(0x400040e370, {0x40001307b0, 0xc}, 0x4000000640)
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/model/indexhandler.go:439 +0x430
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/model.(*model).ccHandleFolders(, {_, _, _}, {{0xdb, 0xa, 0xa4, 0x47, 0xc6, 0x6f, ...}, ...}, ...)
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/model/model.go:1496 +0x5a8
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/model.(*model).ClusterConfig(0x40001c6180, {0x637d8d4148, 0x4000eec350}, {{0x400154ca00?, 0x40000e8088?, 0x40000e8088?}, 0x68?})
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/model/model.go:1296 +0x784
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol.(*connectionWrappingModel).ClusterConfig(0x4000bc5d68?, {{0x400154ca00?, 0x10?, 0x637d710540?}, 0x1?})
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol/protocol.go:1143 +0x50
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol.encryptedModel.ClusterConfig(...)
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol/encryption.go:155
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol.(*rawConnection).dispatcherLoop(0x4000116a20)
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol/protocol.go:496 +0x308
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol.(*rawConnection).Start.func2()
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol/protocol.go:307 +0x24
11-19 11:32:51.663 21476 0 E Go : created by github.com/syncthing/syncthing/lib/protocol.(*rawConnection).Start
11-19 11:32:51.663 21476 0 E Go : github.com/syncthing/syncthing/lib/protocol/protocol.go:306 +0x144
11-19 11:32:51.663 21476 0 E Go :

There's recently changed/added code that sets up index handlers and access fmut protected fields without holding the lock: https://github.com/syncthing/syncthing/blob/main/lib/model/model.go#L1370
I don't see how exactly this can lead to the panic, but presumably some race around pausing a folder.

I am sending a PR shortly.
@imsodin imsodin added bug A problem with current functionality, as opposed to missing functionality (enhancement) needs-triage New issues needed to be validated and removed needs-triage New issues needed to be validated labels Nov 19, 2023
imsodin added a commit that referenced this issue Nov 19, 2023
@imsodin
lib/model: Acquire fmut lock in ensureIndexHandler (fixes #9234)
75d5684 
Towards the end of the function f.folderCfgs and f.folderRunners are
read without the lock.
@calmh calmh closed this as completed in 958ff67 Nov 20, 2023
calmh added a commit to calmh/syncthing that referenced this issue Dec 5, 2023
@calmh
Merge branch 'main' into androidfatdst
48ec761 
* main:
  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
  cmd/ursrv: Add metrics for incoming reports
  gui, man, authors: Update docs, translations, and contributors
  gui: Specialize a special-purpose checkbox style (syncthing#9236)
  gui, man, authors: Update docs, translations, and contributors
  build: Support new nested namespaces in Weblate downloads
  lib/model: Acquire fmut lock in ensureIndexHandler (fixes syncthing#9234) (syncthing#9235)
  gui: Allow to translate "unknown device" (syncthing#9229)
@calmh calmh mentioned this issue Dec 7, 2023
Closed
@calmh calmh added this to the v1.27.0 milestone Dec 7, 2023
@imsodin imsodin reopened this Dec 7, 2023
@imsodin
Copy link
Member Author

imsodin commented Dec 7, 2023

Unfortunately this is still happening in 1.27.0, as reported here:
https://forum.syncthing.net/t/syncthing-1-26-crashing-on-android-in-syncthing-fork/21115/12

Same panic.

I already have a new thing to fix :)

imsodin added a commit to imsodin/syncthing that referenced this issue Dec 7, 2023
@imsodin
lib/model: Remove runner during folder cleanup (fixes syncthing#9234)
3fc3926 
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
Copy link
Member Author

imsodin commented Dec 7, 2023

New issue is where we track this, this stays closed :)
#9269

@imsodin imsodin closed this as completed Dec 7, 2023
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.0
Development

No branches or pull requests
2 participants
@calmh @imsodin