Repository: syncthing/syncthing (language: go)

Issue:
Filenames with extended characters not ignored correctly on macOS
### What happened?

With `.stignore` content:

```
bäd
bad
```
The file `bad` is ignored by `bäd` is not.

I also found a workaround. If you use .stignore` content:

```
ba*d
```
it matches and ignores both files. Possibly `ba?d` would also match, but I have tested it.

I believe this is because the NFD-encoded paths (on macOS) are being matched against NFC-encoded ignore patterns.

The solution is to convert to NFC-encoding before matching.

I have confirmed the bug and a proposed fix by modifying the existing unit test.

Note that some non-ASCII characters work, I presume those that have identical NFC and NFD form.

PR forthcoming.

### Syncthing version

v1.27.8

### Platform & operating system

macOS (64-bit ARM)

### Browser version

_No response_

### Relevant log output

_No response_
Fix handling of extended characters in .stignore on macOS
### Purpose

Normalize pathnames before comparing to ignore patterns. (issue #9597).

Added to the existing unit test. The existing unit tests covers extended character patterns, but it is when a NFD filename is matched against a NFC pattern that the an expected match did not occur.

Made the unit test more robust by passing in native-format pathnames every time Matcher.Match is invoked, to uncover potential future similar bugs.

### Testing

Updated unit test fails without fix, passes with fix. (On macOS at least).

Benchmark results:

```
Before:
BenchmarkMatch-8                 2180224               538.7 ns/op
BenchmarkMatchCached-8           9932952               119.6 ns/op

BenchmarkMatch-8                 2192263               534.9 ns/op
BenchmarkMatchCached-8           9533098               119.8 ns/op

BenchmarkMatch-8                 2171739               536.4 ns/op
BenchmarkMatchCached-8           9956037               120.5 ns/op

BenchmarkMatch-8                 2192551               538.2 ns/op
BenchmarkMatchCached-8           9385444               121.7 ns/op

BenchmarkMatch-8                 2140863               539.8 ns/op
BenchmarkMatchCached-8           9518190               122.1 ns/op

BenchmarkMatch-8                 2189522               531.4 ns/op
BenchmarkMatchCached-8           9751231               118.8 ns/op

BenchmarkMatch-8                 2192011               536.6 ns/op
BenchmarkMatchCached-8           9524190               119.8 ns/op


After:
BenchmarkMatch-8                 2164195               540.4 ns/op
BenchmarkMatchCached-8           9753216               119.9 ns/op

BenchmarkMatch-8                 2177550               540.8 ns/op
BenchmarkMatchCached-8           9671676               120.2 ns/op

BenchmarkMatch-8                 2171844               541.7 ns/op
BenchmarkMatchCached-8           9651882               119.6 ns/op

BenchmarkMatch-8                 2164958               540.8 ns/op
BenchmarkMatchCached-8           9804344               119.4 ns/op

BenchmarkMatch-8                 2141856               542.0 ns/op
BenchmarkMatchCached-8           9493785               120.3 ns/op

BenchmarkMatch-8                 2168912               539.3 ns/op
BenchmarkMatchCached-8           9555124               120.2 ns/op

BenchmarkMatch-8                 2171197               540.3 ns/op
BenchmarkMatchCached-8           9690603               119.6 ns/op
```

This shows a 0.87% mean performance degradation on `BenchmarkMatch`

I have considered how to minimise this. The normalisation could be applied only if the any of the ignore patterns contain any extended characters (detected by whether NFC-form matches NFD-form). Interested in others' thoughts on whether this needs to be optimised?

### Screenshots

N/A

### Documentation

N/A

## Authorship

Your name and email will be added automatically to the AUTHORS file
based on the commit metadata.


Filenames with extended characters not ignored correctly on macOS
### What happened?

With `.stignore` content:

```
bäd
bad
```
The file `bad` is ignored by `bäd` is not.

I also found a workaround. If you use .stignore` content:

```
ba*d
```
it matches and ignores both files. Possibly `ba?d` would also match, but I have tested it.

I believe this is because the NFD-encoded paths (on macOS) are being matched against NFC-encoded ignore patterns.

The solution is to convert to NFC-encoding before matching.

I have confirmed the bug and a proposed fix by modifying the existing unit test.

Note that some non-ASCII characters work, I presume those that have identical NFC and NFD form.

PR forthcoming.

### Syncthing version

v1.27.8

### Platform & operating system

macOS (64-bit ARM)

### Browser version

_No response_

### Relevant log output

_No response_


Discussion / hints:

Looks reasonable to me. The bulk of the changes to the test code are unnecessary though, calling NativeFilename on hard coded ASCII strings.
> Looks reasonable to me. The bulk of the changes to the test code are unnecessary though, calling NativeFilename on hard coded ASCII strings.

Yeah I went back and forth about whether to update them all. Yes they're empriically unnecessary, though technically correct, and help protect against hypothetical future bugs where native/normalised form could have other variations. And there's a cost to test readability.

I deliberately put this in a separate commit specifically so it can be reverted if you prefer. I don't object either way.
My gut feeling is that this is the wrong way around, and we should denormalize/nativefy the ignore patterns instead, however I might be confusing things so let me try to explain (or showcase the confusion):

BEP is normalized, however we convert everything to native format - so any strings coming from the DB (file infos, ...) should be NFD on macos. Macos filesystem uses NFD so anything from fs interactions is NFD. So my initial confusion is: Where does NFC come into this? Some reading (very inconclusively) suggest not everything is NFD, just filesystem. So I presume text entered through the browser/gui/api ends up as NFC in our ignore file. Same for text editors possibly (tested that bash shenanigans on macos produce an NFC "ä").  
Anyway, given we use NFD for all file info in DB, it seems like it would make sense to use NFD for (parsed) ignore patterns as well. Probably call `osutil.NativeFilename` early in `parseLine`.

Does that make any sense or can you point out where I confuse things? :) 
Thanks @imsodin. I had considered this too, as it would also be more performant, but I was worried that, as I understand, not all filestems enforce filenames being normalised (I found some historical syncthing issue around ZFS when I was researching).

Yes, I understand that all text entered in .stignore will be NFC.

But if we can be sure that any pathnames passed in to the matching logic are **normalized** native format, then I agree with your suggestion.

Will research further.
Oh I by no means am sure that anything coming from FS is NFD normalised. I just know that syncthing internal data/strings/filenames are nfd. so a minimal fix for the issue you describe seems to be to ensure ignores are also nfd. if it then turns out that some fs aren't normalised, that seems like an additional, somewhat independent issue.
Iirc we don't actually normalise for internal work, we just normalise on the wire and in the database. I think the solution here is proper, as far as I understand how it's supposed to work.

(Maybe we should always use nfc or whatever internally and just convert at the outermost layer, though.)
> Maybe we should always use nfc or whatever internally and just convert at the outermost layer, though.

Yeah. This whole normalization thing is rather expensive.
FYI This was the issue re ZFS normaliszation: #4649 
> (Maybe we should always use nfc or whatever internally and just convert at the outermost layer, though.)

Afaik that's what we do already (modulo the stuff I missed):

1. Wire/protocol  
Everything going to the wire is NFC normalized. On macos it gets converted to NFD from the wire, on the rest left alone (so still NFC).
2. Filesystem  
On the filesystem layer itself afaik we don't do anything about normalization. However while scanning/putting things into DB we do normalize - NFD on macos, NFC everywhere else.

I don't see any other points of entry for filenames, are there?

So from the above I'd think we are able to trust that input to ignores is normalized. Looking at the scanning code a bit closer though shows that's not the case: We test "ignoredness" before normalization. However it seems like fixing/changing that, and then just deal with normalized filenames only internally seems cleaner than re-normalize whereever we need to be sure things are normalized (with "whereever" == ignores right now, slight hyperbole there :P ).

One more place we currently don't trust normalization either: Protocol to the wire. We use both `ToSlash` and `norm.NFC` regardless of platform to the wire. That we do both however makes me think that we are just extra safe there, not that we have actual reason to believe it changes anything (on unixes).


Fix patch:
diff --git a/lib/ignore/ignore.go b/lib/ignore/ignore.go
index f0510d82540..c564a0a5f9b 100644
--- a/lib/ignore/ignore.go
+++ b/lib/ignore/ignore.go
@@ -18,7 +18,9 @@ import (
 	"time"
 
 	"github.com/gobwas/glob"
+	"golang.org/x/text/unicode/norm"
 
+	"github.com/syncthing/syncthing/lib/build"
 	"github.com/syncthing/syncthing/lib/fs"
 	"github.com/syncthing/syncthing/lib/ignore/ignoreresult"
 	"github.com/syncthing/syncthing/lib/osutil"
@@ -212,6 +214,11 @@ func (m *Matcher) parseLocked(r io.Reader, file string) error {
 }
 
 // Match matches the patterns plus temporary and internal files.
+//
+// The "file" parameter must be in the OS' native unicode format (NFD on macos,
+// NFC everywhere else). This is always the case in real usage in syncthing, as
+// we ensure native unicode normalisation on all entry points (scanning and from
+// protocol) - so no need to normalize when calling this, except e.g. in tests.
 func (m *Matcher) Match(file string) (result ignoreresult.R) {
 	switch {
 	case fs.IsTemporary(file):
@@ -387,6 +394,10 @@ func loadParseIncludeFile(filesystem fs.Filesystem, file string, cd ChangeDetect
 }
 
 func parseLine(line string) ([]Pattern, error) {
+	// We use native normalization internally, thus the patterns must match
+	// that to avoid false negative matches.
+	line = nativeUnicodeNorm(line)
+
 	pattern := Pattern{
 		result: ignoreresult.Ignored,
 	}
@@ -464,6 +475,13 @@ func parseLine(line string) ([]Pattern, error) {
 	return patterns, nil
 }
 
+func nativeUnicodeNorm(s string) string {
+	if build.IsDarwin || build.IsIOS {
+		return norm.NFD.String(s)
+	}
+	return norm.NFC.String(s)
+}
+
 func parseIgnoreFile(fs fs.Filesystem, fd io.Reader, currentFile string, cd ChangeDetector, linesSeen map[string]struct{}) ([]string, []Pattern, error) {
 	var patterns []Pattern
 
diff --git a/lib/scanner/walk.go b/lib/scanner/walk.go
index 3c34ca28546..7146f9ae066 100644
--- a/lib/scanner/walk.go
+++ b/lib/scanner/walk.go
@@ -293,6 +293,11 @@ func (w *walker) walkAndHashFiles(ctx context.Context, toHashChan chan<- protoco
 			return skip
 		}
 
+		// Just in case the filesystem doesn't produce the normalization the OS
+		// uses, and we use internally.
+		nonNormPath := path
+		path = normalizePath(path)
+
 		if m := w.Matcher.Match(path); m.IsIgnored() {
 			l.Debugln(w, "ignored (patterns):", path)
 			// Only descend if matcher says so and the current file is not a symlink.
@@ -319,9 +324,23 @@ func (w *walker) walkAndHashFiles(ctx context.Context, toHashChan chan<- protoco
 			return nil
 		}
 
+		if path != nonNormPath {
+			if !w.AutoNormalize {
+				// We're not authorized to do anything about it, so complain and skip.
+				handleError(ctx, "normalizing path", nonNormPath, errUTF8Normalization, finishedChan)
+				return skip
+			}
+
+			path, err = w.applyNormalization(nonNormPath, path, info)
+			if err != nil {
+				handleError(ctx, "normalizing path", nonNormPath, err, finishedChan)
+				return skip
+			}
+		}
+
 		if ignoredParent == "" {
 			// parent isn't ignored, nothing special
-			return w.handleItem(ctx, path, info, toHashChan, finishedChan, skip)
+			return w.handleItem(ctx, path, info, toHashChan, finishedChan)
 		}
 
 		// Part of current path below the ignored (potential) parent
@@ -330,7 +349,7 @@ func (w *walker) walkAndHashFiles(ctx context.Context, toHashChan chan<- protoco
 		// ignored path isn't actually a parent of the current path
 		if rel == path {
 			ignoredParent = ""
-			return w.handleItem(ctx, path, info, toHashChan, finishedChan, skip)
+			return w.handleItem(ctx, path, info, toHashChan, finishedChan)
 		}
 
 		// The previously ignored parent directories of the current, not
@@ -345,7 +364,7 @@ func (w *walker) walkAndHashFiles(ctx context.Context, toHashChan chan<- protoco
 				handleError(ctx, "scan", ignoredParent, err, finishedChan)
 				return skip
 			}
-			if err = w.handleItem(ctx, ignoredParent, info, toHashChan, finishedChan, skip); err != nil {
+			if err = w.handleItem(ctx, ignoredParent, info, toHashChan, finishedChan); err != nil {
 				return err
 			}
 		}
@@ -355,14 +374,7 @@ func (w *walker) walkAndHashFiles(ctx context.Context, toHashChan chan<- protoco
 	}
 }
 
-func (w *walker) handleItem(ctx context.Context, path string, info fs.FileInfo, toHashChan chan<- protocol.FileInfo, finishedChan chan<- ScanResult, skip error) error {
-	oldPath := path
-	path, err := w.normalizePath(path, info)
-	if err != nil {
-		handleError(ctx, "normalizing path", oldPath, err, finishedChan)
-		return skip
-	}
-
+func (w *walker) handleItem(ctx context.Context, path string, info fs.FileInfo, toHashChan chan<- protocol.FileInfo, finishedChan chan<- ScanResult) error {
 	switch {
 	case info.IsSymlink():
 		if err := w.walkSymlink(ctx, path, info, finishedChan); err != nil {
@@ -375,13 +387,13 @@ func (w *walker) handleItem(ctx context.Context, path string, info fs.FileInfo,
 		return nil
 
 	case info.IsDir():
-		err = w.walkDir(ctx, path, info, finishedChan)
+		return w.walkDir(ctx, path, info, finishedChan)
 
 	case info.IsRegular():
-		err = w.walkRegular(ctx, path, info, toHashChan)
+		return w.walkRegular(ctx, path, info, toHashChan)
 	}
 
-	return err
+	return fmt.Errorf("bug: file info for %v is neither symlink, dir nor regular", path)
 }
 
 func (w *walker) walkRegular(ctx context.Context, relPath string, info fs.FileInfo, toHashChan chan<- protocol.FileInfo) error {
@@ -550,30 +562,21 @@ func (w *walker) walkSymlink(ctx context.Context, relPath string, info fs.FileIn
 	return nil
 }
 
-// normalizePath returns the normalized relative path (possibly after fixing
-// it on disk), or skip is true.
-func (w *walker) normalizePath(path string, info fs.FileInfo) (normPath string, err error) {
+func normalizePath(path string) string {
 	if build.IsDarwin || build.IsIOS {
 		// Mac OS X file names should always be NFD normalized.
-		normPath = norm.NFD.String(path)
-	} else {
-		// Every other OS in the known universe uses NFC or just plain
-		// doesn't bother to define an encoding. In our case *we* do care,
-		// so we enforce NFC regardless.
-		normPath = norm.NFC.String(path)
-	}
-
-	if path == normPath {
-		// The file name is already normalized: nothing to do
-		return path, nil
-	}
-
-	if !w.AutoNormalize {
-		// We're not authorized to do anything about it, so complain and skip.
-
-		return "", errUTF8Normalization
+		return norm.NFD.String(path)
 	}
+	// Every other OS in the known universe uses NFC or just plain
+	// doesn't bother to define an encoding. In our case *we* do care,
+	// so we enforce NFC regardless.
+	return norm.NFC.String(path)
+}
 
+// applyNormalization fixes the normalization of the file on disk, i.e. ensures
+// the file at path ends up named normPath. It shouldn't but may happen that the
+// file ends up with a different name, in which case that one should be scanned.
+func (w *walker) applyNormalization(path, normPath string, info fs.FileInfo) (string, error) {
 	// We will attempt to normalize it.
 	normInfo, err := w.Filesystem.Lstat(normPath)
 	if fs.IsNotExist(err) {


Test patch:
diff --git a/lib/ignore/ignore_test.go b/lib/ignore/ignore_test.go
index 5d0a8b02cb4..72a0c4db70f 100644
--- a/lib/ignore/ignore_test.go
+++ b/lib/ignore/ignore_test.go
@@ -805,7 +805,10 @@ func TestIssue3174(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if !pats.Match("åäö").IsIgnored() {
+	// The pattern above is normalized when parsing, and in order for this
+	// string to match the pattern, it needs to use the same normalization. And
+	// Go always uses NFC regardless of OS, while we use NFD on macos.
+	if !pats.Match(nativeUnicodeNorm("åäö")).IsIgnored() {
 		t.Error("Should match")
 	}
 }