error when sync dest to nonexistant root directory on windows

wwwutz · February 12, 2018, 4:34pm

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

any hints what I’m doing wrong ?

AudriusButkevicius · February 12, 2018, 9:10pm

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.

wwwutz · February 13, 2018, 7:49am

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

if [ ! -d F:\FOO ] then F:\FOO\BAR failes with "... mkdir \\?:"
if [ -d F:\FOO ] then F:\FOO\BAR gets created
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.

calmh · February 13, 2018, 8:25am

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.

wwwutz · February 13, 2018, 12:56pm

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 ?!

AudriusButkevicius · February 13, 2018, 1:18pm

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

wwwutz · February 13, 2018, 4:27pm

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.

AudriusButkevicius · February 13, 2018, 7:44pm

Why would rel be the full path?

wwwutz · February 14, 2018, 8:40am

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

AudriusButkevicius · February 14, 2018, 9:06am

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

AudriusButkevicius · February 14, 2018, 9:06am

No, actually it’s ok

AudriusButkevicius · February 14, 2018, 9:07am

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

calmh · February 14, 2018, 9:24am

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

wwwutz · February 14, 2018, 10:13am

I issued Fails to create folder root directory on Windows · Issue #4762 · syncthing/syncthing · GitHub

imsodin · February 14, 2018, 12:04pm

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).

wwwutz · February 14, 2018, 12:54pm

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

gist.github.com

https://gist.github.com/wwwutz/16656bd2fd26b44a47961c0243a6f319

MkDirAll-test.go

package main

import "fmt"
import "os"

func main() {

	path := `F:\BOOM`
	dir, err := os.Stat(path)

This file has been truncated. show original

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

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>

imsodin · February 14, 2018, 1:15pm

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

wwwutz · February 14, 2018, 1:28pm

nice

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>

wwwutz · February 14, 2018, 1:34pm

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

imsodin · February 14, 2018, 1:57pm

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

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
 			}