error when sync dest to nonexistant root directory on windows


(Peter Marquardt) #1

TL;DR: New folder path X:\ABC\DEF; ABS has to exist, otherwise error.

Setup:

  • Syncthing on windows v0.14.44+38-g2bbd2d6e-dirty
  • Android client (syncthing v0.14.43) add device windows
  • client shares some DCIM folder
  • Windows Syncthing receives "Android wants to share folder "DCIM read-only" (w62ft-9lavg). Add new folder?"
  • Be nice, confirm "Add"
  • WinST: "Add Folder"
  • edit "Folder Path" , replace default with "F:\FOO\BAR"
  • ( there is no F:\FOO ! ) "Save"
  • Folder pops up listed as “Stopped”
  • Eror window pops up:
2018-02-12 17:23:02: Creating directory for "DCIM read-only" (w62ft-9lavg): mkdir \\?: The filename, directory name, or volume label syntax is incorrect

rewind.

  • mkdir F:\FOO
  • remove folder settings from syncthing
  • restart Windows syncthing
  • wait for "wants to share" , “Add”
  • edit "Folder Path" , replace default with "F:\FOO\BAR"
  • Now we actually have a F:\FOO folder. "Save"
  • F:\FOO\BAR gets created by syncthing.exe
  • all is fine.

I’m quite confused about mkdir \\?, since it points to f.Description in lib/config/folderconfiguration.go … which is not very descriptive :sunglasses:

any hints what I’m doing wrong ?


(Audrius Butkevicius) #2

Yes, the path up to where the folder is has to exist, and if it doesn’t error is expected. Yet in the error it seems that the path was not what you expected, and was partially empty (perhaps auto-completion didn’t work or something like that).

Anyways, it seems you resolved your issue, so not sure what the question is.


(Peter Marquardt) #3

Sorry, looks like I wasn’t detailed enough.

  • The error message "... mkdir \\? ..." is wrong. Should be either "mkdir F:\FOO\BAR failed." or "F:\FOO does not exist. mkdir failed."
  • I’m worried about '\\?' in f.Description, since I cannot tell where it comes from ( golang noob here )
  • There is no autocompletion involved anywhere
  1. if [ ! -d F:\FOO ] then F:\FOO\BAR failes with "... mkdir \\?:"
  2. if [ -d F:\FOO ] then F:\FOO\BAR gets created
  3. if [ -d F:\FOO ] then F:\FOO\BAR\BOOM\BANG gets created, if supplied as Folder Path

So the only precondition for the path is the first directory has to exist. all decendants get created. This is likely a bug.


(Jakob Borg) #4

The \\? thing is to avoid Windows filename length limitations. There is a bug in Go standard MkdirAll for paths that begin with \\? so we have our own to work around exactly that. Yet it seems it does not work for you. I don’t know why.

This would be a new limitation, we’ve never required that before? If lib/fs enforces that I guess we can have that as a limitation, but the error doesn’t look like that’s what happening.


(Peter Marquardt) #5

ok, this can be reproduced by just adding a folder.

"+Add Folder" enter F:\FOO\BAR and off you go.

In lib/config/folderconfiguration.go there is a CreateRoot() but f.Path ( which holds F:\FOO\BAR ) is ignored.

Even in lib/model/model.go in CommitConfiguration() we only find CreateRoot() on cfg.Pause() == true, but not false.

It might be some MkdirAll() for the markers which create the folders… and fail due to a MkdirAll bug ?!


(Audrius Butkevicius) #6

f.Path is something that is passed to instantiation of the filesystem. MkdirAll is called on “.” which implies f.Path.


(Peter Marquardt) #7

the culprit seems to be in lib/fs/basicfs.go rooted().

put a debug print there and found there is the situation, where f.root=\\?\F:\FOO and rel=F:\FOO

this gets joined by

joined := filepath.Join(f.root, rel)

resulting in a mkdirAll call with path=\\?\F:\FOO\F:\FOO (!!!)

which obviously fails.


(Audrius Butkevicius) #8

Why would rel be the full path?


(Peter Marquardt) #9

oh, even better, there is a call to os.MkdirAll( '\\?\F:\BOOM', permissions ) with \\?\ prefixed. I tried this in a simple helloworld.go and it failed.

diff to debug (forgive me):

$ git diff
diff --git a/lib/fs/basicfs_windows.go b/lib/fs/basicfs_windows.go
index c4b56de0..b053afdf 100644
--- a/lib/fs/basicfs_windows.go
+++ b/lib/fs/basicfs_windows.go
@@ -16,6 +16,7 @@ import (
        "path/filepath"
        "syscall"
        "unsafe"
+       "runtime"
 )

 var errNotSupported = errors.New("symlinks not supported")
@@ -37,7 +38,13 @@ func (BasicFilesystem) CreateSymlink(target, name string) error {
 // The permission bits perm are used for all directories that MkdirAll creates.
 // If path is already a directory, MkdirAll does nothing and returns nil.
 func (f *BasicFilesystem) MkdirAll(path string, perm FileMode) error {
+       fpcs := make([]uintptr, 1)
+       runtime.Callers(3, fpcs)
+       l.Warnf("##### in f.MkdirAll( '%v' , %v ) from %v", path, perm, runtime.FuncForPC(fpcs[0]-1).Name())
+
+       l.Warnf("##### in f.MkdirAll pre-rooted  f.root='%v' path='%v'", f.root, path)
        path, err := f.rooted(path)
+       l.Warnf("##### in f.MkdirAll post-rooted f.root='%v' path='%v'", f.root, path)
        if err != nil {
                return err
        }
@@ -48,6 +55,7 @@ func (f *BasicFilesystem) MkdirAll(path string, perm FileMode) error {
 // Required due to https://github.com/golang/go/issues/10900
 func (f *BasicFilesystem) mkdirAll(path string, perm os.FileMode) error {
        // Fast path: if we can tell whether path is a directory or file, stop with success or error.
+       l.Warnf("##### in f.mkdirAll( '%v' , %v )", path, perm)
        dir, err := os.Stat(path)
        if err == nil {
                if dir.IsDir() {
@@ -75,7 +83,9 @@ func (f *BasicFilesystem) mkdirAll(path string, perm os.FileMode) error {
                // Create parent
                parent := path[0 : j-1]
                if parent != filepath.VolumeName(parent) {
+       l.Warnf("##### call os.MkdirAll( '%v' , %v )", parent, perm)
                        err = os.MkdirAll(parent, perm)
+       l.Warnf("##### called os.MkdirAll( '%v' , %v ) = %v", parent, perm, err)
                        if err != nil {
                                return err
                        }

resulting log: ( without F:\BOOM folder )

[...]
[56WHJ] 09:32:33 INFO: Ready to synchronize "Default Folder" (default) (readwrite)
[56WHJ] 09:32:33 WARNING: ##### in f.MkdirAll( '.' , 448 ) from github.com/syncthing/syncthing/lib/config.(*FolderConfiguration).CreateRoot
[56WHJ] 09:32:33 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\BOOM\BANG' path='.'
[56WHJ] 09:32:33 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\BOOM\BANG' path='\\?\F:\BOOM\BANG\'
[56WHJ] 09:32:33 WARNING: ##### in f.mkdirAll( '\\?\F:\BOOM\BANG\' , -rwx------ )
[56WHJ] 09:32:33 WARNING: ##### call os.MkdirAll( '\\?\F:\BOOM' , -rwx------ )
[56WHJ] 09:32:33 WARNING: ##### called os.MkdirAll( '\\?\F:\BOOM' , -rwx------ ) = mkdir \\?: The filename, directory name, or volume label syntax is incorrect.
[56WHJ] 09:32:33 WARNING: Creating directory for essu7-czvuv: mkdir \\?: The filename, directory name, or volume label syntax is incorrect.
[56WHJ] 09:32:33 WARNING: Creating folder marker: folder path missing
[56WHJ] 09:32:33 INFO: Ready to synchronize essu7-czvuv (readwrite)
[...]

$ go version
go version go1.9.4 windows/amd64


(Audrius Butkevicius) #10

Pls raise an issue on github, as this doesn’t seem right.


(Audrius Butkevicius) #11

No, actually it’s ok


(Audrius Butkevicius) #12

Seems the issue is in os.MkdirAll not being able to handle UNC paths. So still, raise an issue.


(Jakob Borg) #13

This is why we have our own mkdirall in the fs package…


(Peter Marquardt) #14

I issued :sunglasses: https://github.com/syncthing/syncthing/issues/4762


(Simon) #15

As Jakob said, this is a known problem in Go, as noted in the comment above the function you linked to: https://github.com/golang/go/issues/10900 That’s why we use our own implementation, which shouldn’t have that problem.

So in the “simple helloworld.go” where you reproduced the failure, replace any os functions with their 1to1 replacement from github.com/syncthing/syncthing/lib/fs and give us the result (and script if successful).


(Peter Marquardt) #16

ok, here’s the q&d test script:

hard coded F:\BOOM

I struggle to replace/test it with the syncthing replacement funcs, coz of lack of golang knowledge. I’m learning though :sunglasses:

This is what happens ( remove F:\BOOM first )

waldi+wwwutz@waldi MINGW64 ~/go/src/github.com/wwwutz/gotest
$ go run mkdir-test.go
1 failed: MkdirAll( \\?\F:\BOOM\BANG , perm ) err = mkdir \\?: The filename, directory name, or volume label syntax is incorrect.
2 failed: MkdirAll( \\?\F:\BOOM , perm ) err = mkdir \\?: The filename, directory name, or volume label syntax is incorrect.
done: MkdirAll( F:\BOOM\BANG , perm ) err = <nil>

Since the last one, without UNC, succeded: let’s try again:

waldi+wwwutz@waldi MINGW64 ~/go/src/github.com/wwwutz/gotest
$ go run mkdir-test.go
Found folder F:\BOOM. Lets see what happens now.
done: MkdirAll( F:\BOOM\BANG , perm ) err = <nil>


(Simon) #17

Maybe something like the following (use it as a base, I probably messed something windows specific up and potentially didn’t target exactly what you intended to reproduce): https://gist.github.com/imsodin/a0bafeac922cdf191093d84f69b31a2b


(Peter Marquardt) #18

nice :sunglasses:

ok with my debug lines and no preexisting F:\FOO

$ go run MkDirAll-test.go
14:23:19 WARNING: ##### in f.MkdirAll( 'FOO' , 777 ) from main.main
14:23:19 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\' path='FOO'
14:23:19 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\' path='\\?\F:\FOO'
14:23:19 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO' , -r----x--x )
14:23:19 WARNING: ##### in f.MkdirAll( 'FOO\BAR' , 777 ) from main.main
14:23:19 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\' path='FOO\BAR'
14:23:19 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\' path='\\?\F:\FOO\BAR'
14:23:19 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO\BAR' , -r----x--x )
14:23:19 WARNING: ##### call os.MkdirAll( '\\?\F:\FOO' , -r----x--x )
14:23:19 WARNING: ##### called os.MkdirAll( '\\?\F:\FOO' , -r----x--x ) = <nil>
14:23:19 WARNING: ##### in f.MkdirAll( '.' , 777 ) from main.main
14:23:19 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\FOO' path='.'
14:23:19 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\FOO' path='\\?\F:\FOO\'
14:23:19 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO\' , -r----x--x )
14:23:19 WARNING: ##### in f.MkdirAll( 'BAR' , 777 ) from main.main
14:23:19 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\FOO' path='BAR'
14:23:19 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\FOO' path='\\?\F:\FOO\BAR'
14:23:19 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO\BAR' , -r----x--x )
14:23:19 WARNING: ##### in f.MkdirAll( 'BAR' , 777 ) from main.main
14:23:19 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\FOO\BAR' path='BAR'
14:23:19 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\FOO\BAR' path='\\?\F:\FOO\BAR\BAR'
14:23:19 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO\BAR\BAR' , -r----x--x )
14:23:19 WARNING: ##### call os.MkdirAll( '\\?\F:\FOO\BAR' , -r----x--x )
14:23:19 WARNING: ##### called os.MkdirAll( '\\?\F:\FOO\BAR' , -r----x--x ) = <nil>

Buit this code creates F:\FOO and then F:\FOO\BAR.

My intend was do do /foo/bar first ( mkdir -p ) .

diff --git a/MkDirAll-test.go b/MkDirAll-test.go
index e61ff1d..4c8e1dc 100644
--- a/MkDirAll-test.go
+++ b/MkDirAll-test.go
@@ -9,7 +9,7 @@ import (
 func main() {
        base := `F:\`
        testFs := fs.NewFilesystem(fs.FilesystemTypeBasic, base)
-       for _, targ := range []string{`FOO`, `FOO\BAR`} {
+       for _, targ := range []string{`FOO\BAR`, `FOO`} {
                if err := testFs.MkdirAll(targ, 777); err != nil {
                        fmt.Printf("Base: %v, target: %v, err: %v", base, targ, err)
                }

Now it fails exactly as in syncthing:

$ go run MkDirAll-test.go
14:26:10 WARNING: ##### in f.MkdirAll( 'FOO\BAR' , 777 ) from main.main
14:26:10 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\' path='FOO\BAR'
14:26:10 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\' path='\\?\F:\FOO\BAR'
14:26:10 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO\BAR' , -r----x--x )
14:26:10 WARNING: ##### call os.MkdirAll( '\\?\F:\FOO' , -r----x--x )
14:26:10 WARNING: ##### called os.MkdirAll( '\\?\F:\FOO' , -r----x--x ) = mkdir \\?: The filename, directory name, or volume label syntax is incorrect.
Base: F:\, target: FOO\BAR, err: mkdir \\?: The filename, directory name, or volume label syntax is incorrect.14:26:10 WARNING: ##### in f.MkdirAll( 'FOO' , 777 ) from main.main
14:26:10 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\' path='FOO'
14:26:10 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\' path='\\?\F:\FOO'
14:26:10 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO' , -r----x--x )
14:26:10 WARNING: ##### in f.MkdirAll( '.' , 777 ) from main.main
14:26:10 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\FOO' path='.'
14:26:10 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\FOO' path='\\?\F:\FOO\'
14:26:10 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO\' , -r----x--x )
14:26:10 WARNING: ##### in f.MkdirAll( 'BAR' , 777 ) from main.main
14:26:10 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\FOO' path='BAR'
14:26:10 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\FOO' path='\\?\F:\FOO\BAR'
14:26:10 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO\BAR' , -r----x--x )
14:26:10 WARNING: ##### call os.MkdirAll( '\\?\F:\FOO' , -r----x--x )
14:26:10 WARNING: ##### called os.MkdirAll( '\\?\F:\FOO' , -r----x--x ) = <nil>
14:26:10 WARNING: ##### in f.MkdirAll( 'BAR' , 777 ) from main.main
14:26:10 WARNING: ##### in f.MkdirAll pre-rooted  f.root='\\?\F:\FOO\BAR' path='BAR'
14:26:10 WARNING: ##### in f.MkdirAll post-rooted f.root='\\?\F:\FOO\BAR' path='\\?\F:\FOO\BAR\BAR'
14:26:10 WARNING: ##### in f.mkdirAll( '\\?\F:\FOO\BAR\BAR' , -r----x--x )
14:26:10 WARNING: ##### call os.MkdirAll( '\\?\F:\FOO\BAR' , -r----x--x )
14:26:10 WARNING: ##### called os.MkdirAll( '\\?\F:\FOO\BAR' , -r----x--x ) = <nil>

(Peter Marquardt) #19

hmmm… testFs.MkdirAll does call os.MkdirAll anyways, so I don’t really see the UNC fix…


(Simon) #20

Yes, but it has a check that is supposed to prevent it from doing that on the UNC drive letter. Can you run it with this additional debug:

--- a/lib/fs/basicfs_windows.go
+++ b/lib/fs/basicfs_windows.go
@@ -74,6 +74,7 @@ func (f *BasicFilesystem) mkdirAll(path string, perm os.FileMode) error {
 	if j > 1 {
 		// Create parent
 		parent := path[0 : j-1]
+		l.Infoln("Volume name check", parent, filepath.VolumeName(parent))
 		if parent != filepath.VolumeName(parent) {
 			err = os.MkdirAll(parent, perm)
 			if err != nil {

Edit: Actually I think I have found the problem, you were right about os.MkdirAll being called anyway. I think we can’t run tests on the build servers with operations at drive letter level ( @calmh ?), so I’ll need you ( @wwwutz ) to test it :wink:

diff --git a/lib/fs/basicfs_windows.go b/lib/fs/basicfs_windows.go
index 145022fdc..b7d2f9676 100644
--- a/lib/fs/basicfs_windows.go
+++ b/lib/fs/basicfs_windows.go
@@ -76,7 +76,7 @@ func (f *BasicFilesystem) mkdirAll(path string, perm os.FileMode) error {
 		parent := path[0 : j-1]
 		l.Infoln("Volume name check", parent, filepath.VolumeName(parent))
 		if parent != filepath.VolumeName(parent) {
-			err = os.MkdirAll(parent, perm)
+			err = f.mkdirAll(parent, perm)
 			if err != nil {
 				return err
 			}