Repository: syncthing/syncthing (language: go)

Issue:
Panic in ignore matching on invalid UTF-8 from filesystem watcher
### Does your log mention database corruption?
No

### Include required information
**Version**: Breaks on `1.27.2` and the current git `main` branch (`45beb28f`)

Hello devs, thanks for all the hard work. I hadn't updated syncthing in a while and I started seeing a panic in my logs after doing a system update. The panic seems to happen after syncthing finishes scanning all my folders.

I am unfortunately not sure how to reproduce the problem at this time. The earliest error log is of version `1.27.2`. Perhaps something to do with the file that syncthing didn't have read permissions on? I attached the logs with personal info removed.
[panic-20240126-012105.reported.log](https://github.com/syncthing/syncthing/files/14058310/panic-20240126-012105.reported.log)

Panic in ignore matching on invalid UTF-8 from filesystem watcher
### Does your log mention database corruption?
No

### Include required information
**Version**: Breaks on `1.27.2` and the current git `main` branch (`45beb28f`)

Hello devs, thanks for all the hard work. I hadn't updated syncthing in a while and I started seeing a panic in my logs after doing a system update. The panic seems to happen after syncthing finishes scanning all my folders.

I am unfortunately not sure how to reproduce the problem at this time. The earliest error log is of version `1.27.2`. Perhaps something to do with the file that syncthing didn't have read permissions on? I attached the logs with personal info removed.
[panic-20240126-012105.reported.log](https://github.com/syncthing/syncthing/files/14058310/panic-20240126-012105.reported.log)



Discussion / hints:
Looks like an ignore pattern not understood by the matching library, what are your ignore patterns?

(Relevant part of the trace, for searchability etc:)

```
Panic at 2024-01-26T01:21:05+03:00
panic: runtime error: slice bounds out of range [:5] with length 4

goroutine 229 [running]:
github.com/gobwas/glob/match.Row.matchAll({{0xc00029a9c0, 0x2, 0x2}, 0x4, {0xc00037eae0, 0x1, 0x1}}, {0xc00027933c, 0x4})
	github.com/gobwas/glob@v0.2.3/match/row.go:37 +0x290
github.com/gobwas/glob/match.Row.Index({{0xc00029a9c0, 0x2, 0x2}, 0x4, {0xc00037eae0, 0x1, 0x1}}, {0xc0002792b8, 0x88})
	github.com/gobwas/glob@v0.2.3/match/row.go:71 +0x129
github.com/gobwas/glob/match.BTree.Match({{0x1372d30, 0xc000fa1940}, {0x1372d68, 0xc00016f0e0}, {0x0, 0x0}, 0x4, 0xffffffffffffffff, 0x0, 0xffffffffffffffff}, ...)
	github.com/gobwas/glob@v0.2.3/match/btree.go:103 +0x17e
github.com/syncthing/syncthing/lib/ignore.(*Matcher).Match(0xc001334990, {0xc0002792b8, 0x88})
	github.com/syncthing/syncthing/lib/ignore/ignore.go:270 +0x442
github.com/syncthing/syncthing/lib/fs.(*BasicFilesystem).Watch.func1({0xc000279290?, 0xc004182ab0?})
	github.com/syncthing/syncthing/lib/fs/basicfs_watch.go:45 +0x62
github.com/syncthing/notify.node.AddDir({{0xc0014a0ff0?, 0xc00146f310?}, 0xc0047ce1b0?, 0xc0047ce1e0?}, 0xc003faba10, 0xc001213540)
	github.com/syncthing/notify@v0.0.0-20210616190510-c6b7342338d2/node.go:91 +0x3af
github.com/syncthing/notify.(*nonrecursiveTree).watchrec(0xc000044360, {{0xc0014a0ff0?, 0xc0000af1a0?}, 0xc0047ce1b0?, 0xc0047ce1e0?}, 0x27?, 0x1000fc6, 0x7b4f9c1bb5b8?)
	github.com/syncthing/notify@v0.0.0-20210616190510-c6b7342338d2/tree_nonrecursive.go:247 +0x274
github.com/syncthing/notify.(*nonrecursiveTree).Watch(0xc000044360, {0xc0014a0ff0?, 0x1?}, 0xc001095140, 0x1a680b0?, {0xc00031f19c, 0x1, 0x1?})
	github.com/syncthing/notify@v0.0.0-20210616190510-c6b7342338d2/tree_nonrecursive.go:183 +0x187
github.com/syncthing/notify.WatchWithFilter(...)
	github.com/syncthing/notify@v0.0.0-20210616190510-c6b7342338d2/notify.go:75
github.com/syncthing/syncthing/lib/fs.(*BasicFilesystem).Watch(0xc00041f940, {0x135fd18, 0x1}, {0x136a9a0?, 0xc001334990}, {0x13719f0?, 0xc00149ab90}, 0x40?)
	github.com/syncthing/syncthing/lib/fs/basicfs_watch.go:47 +0x1b6
github.com/syncthing/syncthing/lib/fs.(*caseFilesystem).Watch(0xc0003bc720, {0x135fd18, 0x1}, {0x136a9a0, 0xc001334990}, {0x13719f0, 0xc00149ab90}, 0x40?)
	github.com/syncthing/syncthing/lib/fs/casefs.go:332 +0x99
github.com/syncthing/syncthing/lib/fs.(*metricsFS).Watch(0xc001311df0, {0x135fd18, 0x1}, {0x136a9a0, 0xc001334990}, {0x13719f0, 0xc00149ab90}, 0x0?)
	github.com/syncthing/syncthing/lib/fs/metrics.go:209 +0xfd
github.com/syncthing/syncthing/lib/model.(*folder).monitorWatch(0xc0011c4400, {0x13719f0, 0xc00149ab90})
	github.com/syncthing/syncthing/lib/model/folder.go:1024 +0x7d7
created by github.com/syncthing/syncthing/lib/model.(*folder).startWatch in goroutine 257
	github.com/syncthing/syncthing/lib/model/folder.go:994 +0x136
```

It indeed was an ignore pattern that caused the panic:
```
// vim swap files
(?d)*.*.sw[a-p]
```
Works normally if I remove this line.
Interesting. That doesn't cause an issue for me. Might there be something in particular with the file names it matches against that cause it?
I found the source of the problem after some investigating. I had a file in one of my folders with the filename `�X!` (don't ask me how, I don't know either) which is apparently invalid utf-8 and causes glob to panic for some patterns. I noticed `Match()` in `ignore.go` gets called with the same file multiple times and the instance originating from [here](https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/model/folder.go#L994C2-L994C13) -> [here](https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/fs/basicfs_watch.go#L45C28-L45C38) leads to a panic.

Should `Matcher.Match()` check for valid utf8 before proceeding? Adding a check there fixes the issue, but I feel like the file should have been filtered out before it gets to that stage, maybe in https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/fs/basicfs_watch.go#L40

Side note, method names in a lot of places are very confusing - many functions imply their parameters are dirs but are called with dir *and* file paths, like [this](https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/fs/basicfs_watch.go#L45C28-L45C38), 
Nice catch. The glob package is known flaky, especially with non-ASCII things, but we're currently a bit tightly tied to it for it to be an easy switch.

It was my impression that we filter quite strictly for valid UTF-8 and don't accept other forms of names, but perhaps not in the watching code.
> It was my impression that we filter quite strictly for valid UTF-8 and don't accept other forms of names, but perhaps not in the watching code.

We pass a function to check things to be ignored or not to the notify library. And that library probably doesn't have any such checks.
https://github.com/syncthing/notify/blob/main/notify.go#L73
I think it would make the most sense to do the utf-8 check in the Match function, since that seems to be the only place in the code where it is relevant because of the glob library. Right now it's invoked from multiple places and at least one of them doesn't do all the necessary sanitization.

That being said, it's a bit unexpected that syncthing silently (unless you're looking at the logs) ignores a file that's not in my ignore list. Regardless of the encoding of the file name, it is still a valid file in my file system and I would prefer syncthing to include it by default instead of ignoring it.
All parts of Syncthing depend on being able to understand the filename encoding, as it needs to change that encoding between systems, as not all systems use the same encoding. Treating it as just an opaque string of bytes doesn't work, unfortunately.
I see, I didn't consider cross-compatibility. I assume you have already considered replacing disallowed characters in the file  names similar to what `lib/fs/util.go` does for Folders or keeping some sort of translation/mapping in the db and decided against it?

Anyhow, would it make sense to repurpose `WindowsInvalidFilename` to filter out all disallowed filenames and use it in the initial walk and monitoring steps to have a more consistent behavior? Correct me if I'm wrong but from what I could tell, it seems other invalid files like `CON`, etc. *are* indexed and pushed, and only fail when they are *pulled* by a Windows client with an error shown on the GUI. The behavior is inconsistent with non utf-8 files, which are just ignored. I'd argue non utf-8 filenames should also be pushed if they can be pulled by the receiving end, or there should at least be an option to bypass the compatibility filters. I'm willing to provide the PR for this if the devs agree to it.
I am a bit confused about the valid UTF-8 requirement - we do unicode normalisation in the protocol layer, but that never fails (doesn't return an error). And that's only in the protocol layer, that e.g. doesn't happen in the scanner. I don't see anything else doing validation from a quick look (very good chance I simply missed it).

So as far as I see there's nothing preventing such an invalid filename to get to the matching library even without filesystem watching. You could test that by disabling watching and seeing if the same panic occurs during scanning. If that's the case, then I think indeed the right thing to do is to add some validation in the matcher library. And probably err on the side of not ignoring it?
The scanner normalises, and there shouldn't be any other ways in for a path.
... That was a very good check I did. First it validates utf-8 (https://github.com/syncthing/syncthing/blob/main/lib/scanner/walk.go#L279) then it normlizes inside `handleItem` (after ignoring). So yeah, seems simple and consistent to also do a `utf8.ValidString` in the fs watcher code to prevent that panic. I'd probably not ignore failures when watching - might be useful to get the error during the scan afterwards to get the users attention. On the other hand the exact same argument could be made to ignore it: Avoid confusing the user, who potentially has no clue what to do with an invalid file anyway xD
I don't quite get the valid utf-8 requirment either, especially when other "invalid" files are handled differently.

From what I could gather in my tests, it's only the monitoring code that's susceptible to a panic - scanning is safe because it skips files with invalid utf-8 before matching, and everything else seems to happen after the scan is complete and *shouldn't* encounter any weird strings as they are ignored. I think it's best to add a guard to `Match()` for good measure and open an issue on the glob repo, *and* change the behavior to include invalid utf-8 files by default instead of ignoring them and handling them similarly to other invalid filenames - at least until glob is updated pattern match them without panicking.

Offending code that doesn't filter invalid utf-8 btw:
https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/fs/basicfs_watch.go#L45
@kylosus  Don't confuse the requirement for valid UTF-8, which is fundamental, with the bug in the glob matcher, which is just a bug. :) Apparently in this case things could slip from the watcher to the glob library and cause a panic there before passing any of our validation stuff, and that's unfortunate, but wouldn't have been a problem otherwise as all the watcher does is trigger the scanner -- which then does the normalisation and validation.

@imsodin but yes the ignore patterns are UTF-8 and the things passed to it need to be as well. We should do an `osutil.NormalizeFilename` or something on the way from the watcher to the ignore matcher...
Looks like an ignore pattern not understood by the matching library, what are your ignore patterns?

(Relevant part of the trace, for searchability etc:)

```
Panic at 2024-01-26T01:21:05+03:00
panic: runtime error: slice bounds out of range [:5] with length 4

goroutine 229 [running]:
github.com/gobwas/glob/match.Row.matchAll({{0xc00029a9c0, 0x2, 0x2}, 0x4, {0xc00037eae0, 0x1, 0x1}}, {0xc00027933c, 0x4})
	github.com/gobwas/glob@v0.2.3/match/row.go:37 +0x290
github.com/gobwas/glob/match.Row.Index({{0xc00029a9c0, 0x2, 0x2}, 0x4, {0xc00037eae0, 0x1, 0x1}}, {0xc0002792b8, 0x88})
	github.com/gobwas/glob@v0.2.3/match/row.go:71 +0x129
github.com/gobwas/glob/match.BTree.Match({{0x1372d30, 0xc000fa1940}, {0x1372d68, 0xc00016f0e0}, {0x0, 0x0}, 0x4, 0xffffffffffffffff, 0x0, 0xffffffffffffffff}, ...)
	github.com/gobwas/glob@v0.2.3/match/btree.go:103 +0x17e
github.com/syncthing/syncthing/lib/ignore.(*Matcher).Match(0xc001334990, {0xc0002792b8, 0x88})
	github.com/syncthing/syncthing/lib/ignore/ignore.go:270 +0x442
github.com/syncthing/syncthing/lib/fs.(*BasicFilesystem).Watch.func1({0xc000279290?, 0xc004182ab0?})
	github.com/syncthing/syncthing/lib/fs/basicfs_watch.go:45 +0x62
github.com/syncthing/notify.node.AddDir({{0xc0014a0ff0?, 0xc00146f310?}, 0xc0047ce1b0?, 0xc0047ce1e0?}, 0xc003faba10, 0xc001213540)
	github.com/syncthing/notify@v0.0.0-20210616190510-c6b7342338d2/node.go:91 +0x3af
github.com/syncthing/notify.(*nonrecursiveTree).watchrec(0xc000044360, {{0xc0014a0ff0?, 0xc0000af1a0?}, 0xc0047ce1b0?, 0xc0047ce1e0?}, 0x27?, 0x1000fc6, 0x7b4f9c1bb5b8?)
	github.com/syncthing/notify@v0.0.0-20210616190510-c6b7342338d2/tree_nonrecursive.go:247 +0x274
github.com/syncthing/notify.(*nonrecursiveTree).Watch(0xc000044360, {0xc0014a0ff0?, 0x1?}, 0xc001095140, 0x1a680b0?, {0xc00031f19c, 0x1, 0x1?})
	github.com/syncthing/notify@v0.0.0-20210616190510-c6b7342338d2/tree_nonrecursive.go:183 +0x187
github.com/syncthing/notify.WatchWithFilter(...)
	github.com/syncthing/notify@v0.0.0-20210616190510-c6b7342338d2/notify.go:75
github.com/syncthing/syncthing/lib/fs.(*BasicFilesystem).Watch(0xc00041f940, {0x135fd18, 0x1}, {0x136a9a0?, 0xc001334990}, {0x13719f0?, 0xc00149ab90}, 0x40?)
	github.com/syncthing/syncthing/lib/fs/basicfs_watch.go:47 +0x1b6
github.com/syncthing/syncthing/lib/fs.(*caseFilesystem).Watch(0xc0003bc720, {0x135fd18, 0x1}, {0x136a9a0, 0xc001334990}, {0x13719f0, 0xc00149ab90}, 0x40?)
	github.com/syncthing/syncthing/lib/fs/casefs.go:332 +0x99
github.com/syncthing/syncthing/lib/fs.(*metricsFS).Watch(0xc001311df0, {0x135fd18, 0x1}, {0x136a9a0, 0xc001334990}, {0x13719f0, 0xc00149ab90}, 0x0?)
	github.com/syncthing/syncthing/lib/fs/metrics.go:209 +0xfd
github.com/syncthing/syncthing/lib/model.(*folder).monitorWatch(0xc0011c4400, {0x13719f0, 0xc00149ab90})
	github.com/syncthing/syncthing/lib/model/folder.go:1024 +0x7d7
created by github.com/syncthing/syncthing/lib/model.(*folder).startWatch in goroutine 257
	github.com/syncthing/syncthing/lib/model/folder.go:994 +0x136
```

It indeed was an ignore pattern that caused the panic:
```
// vim swap files
(?d)*.*.sw[a-p]
```
Works normally if I remove this line.
Interesting. That doesn't cause an issue for me. Might there be something in particular with the file names it matches against that cause it?
I found the source of the problem after some investigating. I had a file in one of my folders with the filename `�X!` (don't ask me how, I don't know either) which is apparently invalid utf-8 and causes glob to panic for some patterns. I noticed `Match()` in `ignore.go` gets called with the same file multiple times and the instance originating from [here](https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/model/folder.go#L994C2-L994C13) -> [here](https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/fs/basicfs_watch.go#L45C28-L45C38) leads to a panic.

Should `Matcher.Match()` check for valid utf8 before proceeding? Adding a check there fixes the issue, but I feel like the file should have been filtered out before it gets to that stage, maybe in https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/fs/basicfs_watch.go#L40

Side note, method names in a lot of places are very confusing - many functions imply their parameters are dirs but are called with dir *and* file paths, like [this](https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/fs/basicfs_watch.go#L45C28-L45C38), 
Nice catch. The glob package is known flaky, especially with non-ASCII things, but we're currently a bit tightly tied to it for it to be an easy switch.

It was my impression that we filter quite strictly for valid UTF-8 and don't accept other forms of names, but perhaps not in the watching code.
> It was my impression that we filter quite strictly for valid UTF-8 and don't accept other forms of names, but perhaps not in the watching code.

We pass a function to check things to be ignored or not to the notify library. And that library probably doesn't have any such checks.
https://github.com/syncthing/notify/blob/main/notify.go#L73
I think it would make the most sense to do the utf-8 check in the Match function, since that seems to be the only place in the code where it is relevant because of the glob library. Right now it's invoked from multiple places and at least one of them doesn't do all the necessary sanitization.

That being said, it's a bit unexpected that syncthing silently (unless you're looking at the logs) ignores a file that's not in my ignore list. Regardless of the encoding of the file name, it is still a valid file in my file system and I would prefer syncthing to include it by default instead of ignoring it.
All parts of Syncthing depend on being able to understand the filename encoding, as it needs to change that encoding between systems, as not all systems use the same encoding. Treating it as just an opaque string of bytes doesn't work, unfortunately.
I see, I didn't consider cross-compatibility. I assume you have already considered replacing disallowed characters in the file  names similar to what `lib/fs/util.go` does for Folders or keeping some sort of translation/mapping in the db and decided against it?

Anyhow, would it make sense to repurpose `WindowsInvalidFilename` to filter out all disallowed filenames and use it in the initial walk and monitoring steps to have a more consistent behavior? Correct me if I'm wrong but from what I could tell, it seems other invalid files like `CON`, etc. *are* indexed and pushed, and only fail when they are *pulled* by a Windows client with an error shown on the GUI. The behavior is inconsistent with non utf-8 files, which are just ignored. I'd argue non utf-8 filenames should also be pushed if they can be pulled by the receiving end, or there should at least be an option to bypass the compatibility filters. I'm willing to provide the PR for this if the devs agree to it.
I am a bit confused about the valid UTF-8 requirement - we do unicode normalisation in the protocol layer, but that never fails (doesn't return an error). And that's only in the protocol layer, that e.g. doesn't happen in the scanner. I don't see anything else doing validation from a quick look (very good chance I simply missed it).

So as far as I see there's nothing preventing such an invalid filename to get to the matching library even without filesystem watching. You could test that by disabling watching and seeing if the same panic occurs during scanning. If that's the case, then I think indeed the right thing to do is to add some validation in the matcher library. And probably err on the side of not ignoring it?
The scanner normalises, and there shouldn't be any other ways in for a path.
... That was a very good check I did. First it validates utf-8 (https://github.com/syncthing/syncthing/blob/main/lib/scanner/walk.go#L279) then it normlizes inside `handleItem` (after ignoring). So yeah, seems simple and consistent to also do a `utf8.ValidString` in the fs watcher code to prevent that panic. I'd probably not ignore failures when watching - might be useful to get the error during the scan afterwards to get the users attention. On the other hand the exact same argument could be made to ignore it: Avoid confusing the user, who potentially has no clue what to do with an invalid file anyway xD
I don't quite get the valid utf-8 requirment either, especially when other "invalid" files are handled differently.

From what I could gather in my tests, it's only the monitoring code that's susceptible to a panic - scanning is safe because it skips files with invalid utf-8 before matching, and everything else seems to happen after the scan is complete and *shouldn't* encounter any weird strings as they are ignored. I think it's best to add a guard to `Match()` for good measure and open an issue on the glob repo, *and* change the behavior to include invalid utf-8 files by default instead of ignoring them and handling them similarly to other invalid filenames - at least until glob is updated pattern match them without panicking.

Offending code that doesn't filter invalid utf-8 btw:
https://github.com/syncthing/syncthing/blob/45beb28fa5b051d14a10f102a05ec46d36ecaa75/lib/fs/basicfs_watch.go#L45
@kylosus  Don't confuse the requirement for valid UTF-8, which is fundamental, with the bug in the glob matcher, which is just a bug. :) Apparently in this case things could slip from the watcher to the glob library and cause a panic there before passing any of our validation stuff, and that's unfortunate, but wouldn't have been a problem otherwise as all the watcher does is trigger the scanner -- which then does the normalisation and validation.

@imsodin but yes the ignore patterns are UTF-8 and the things passed to it need to be as well. We should do an `osutil.NormalizeFilename` or something on the way from the watcher to the ignore matcher...

Fix patch:
diff --git a/lib/fs/basicfs_watch.go b/lib/fs/basicfs_watch.go
index ca3f306d3fb..c65529cc50c 100644
--- a/lib/fs/basicfs_watch.go
+++ b/lib/fs/basicfs_watch.go
@@ -14,6 +14,7 @@ package fs
 import (
 	"context"
 	"errors"
+	"unicode/utf8"
 
 	"github.com/syncthing/notify"
 )
@@ -38,6 +39,10 @@ func (f *BasicFilesystem) Watch(name string, ignore Matcher, ctx context.Context
 	}
 
 	absShouldIgnore := func(absPath string) bool {
+		if !utf8.ValidString(absPath) {
+			return true
+		}
+
 		rel, err := f.unrootedChecked(absPath, roots)
 		if err != nil {
 			return true
@@ -78,7 +83,14 @@ func (f *BasicFilesystem) watchLoop(ctx context.Context, name string, roots []st
 
 		select {
 		case ev := <-backendChan:
-			relPath, err := f.unrootedChecked(ev.Path(), roots)
+			evPath := ev.Path()
+
+			if !utf8.ValidString(evPath) {
+				l.Debugln(f.Type(), f.URI(), "Watch: Ignoring invalid UTF-8")
+				continue
+			}
+
+			relPath, err := f.unrootedChecked(evPath, roots)
 			if err != nil {
 				select {
 				case errChan <- err:


Test patch: