CC @alexbrainman

I won't be able to look into until I am back home (next week). I am also concerned that I won't be able to reproduce the problem, because, perhaps, I will need "Windows Server 2016" (which I don't have).

Built a version of go-swagger, and run it Panic's when whilst compiling a swagger.json.

How can I do that? What are the steps? Than you.

Alex

I will try to re-produce the crash scenario on Windows 10, however to see some of the problem. Here are the tools I'm using to create test reparse points :

Junction.exe published by sysinternals, available from Microsoft at : https://technet.microsoft.com/en-us/sysinternals/bb896768

fsutil.exe, which ships as part of the O.S., described here: https://technet.microsoft.com/en-us/library/cc753059(v=ws.11).aspx

Note the output produced by the following program:

package main

import (
"flag"
"fmt"
"os"
)

func main() {

dir := flag.String("d", "", "The Directory to be tested")
flag.Parse()
if *dir != "" {
    results, err := os.Readlink(*dir)
    fmt.Printf("Readlink of %s produced %#v and error %v", *dir, results, err)
}

}

PS E:\temp> .\Junction\junction.exe goes_somewhere .\somewhere

Junction v1.06 - Windows junction creator and reparse point viewer
Copyright (C) 2000-2010 Mark Russinovich
Sysinternals - www.sysinternals.com

Created: E:\temp\goes_somewhere
Targetted at: E:\temp\somewhere

PS E:\temp> fsutil reparsepoint query .\goes_somewhere
Reparse Tag Value : 0xa0000003
Tag value: Microsoft
Tag value: Name Surrogate
Tag value: Mount Point
Substitue Name offset: 0
Substitue Name length: 42
Print Name offset: 44
Print Name Length: 0
Substitute Name: ??\E:\temp\somewhere

Reparse Data Length: 0x00000036
Reparse Data:
0000: 00 00 2a 00 2c 00 00 00 5c 00 3f 00 3f 00 5c 00 ..*.,....?.?..
0010: 45 00 3a 00 5c 00 74 00 65 00 6d 00 70 00 5c 00 E.:..t.e.m.p..
0020: 73 00 6f 00 6d 00 65 00 77 00 68 00 65 00 72 00 s.o.m.e.w.h.e.r.
0030: 65 00 00 00 c4 f2 e.....

PS E:\temp> C:\users\nkaethler\s\p4\dev\src\Chroma\Go\bin\junction.exe -d goes_somewhere
Readlink of goes_somewhere produced "\uf2c4" and error

PS E:\temp> dir

Directory: E:\temp

Mode LastWriteTime Length Name

d----- 6/7/2016 11:17 AM a
d----l 6/7/2016 11:23 AM goes_nowhere
d----l 6/7/2016 11:36 AM goes_somewhere
d----- 6/7/2016 11:22 AM Junction
d----- 6/7/2016 11:35 AM somewhere
-a---l 6/7/2016 11:16 AM 20 b.txt
-a---- 6/7/2016 11:21 AM 79623 Junction.zip

PS E:\temp>

Here is a sample of what fsutil reparsepoint query shows on Windows Server 2016 TP4 :

Windows PowerShell
Copyright (C) 2015 Microsoft Corporation. All rights reserved.

PS C:\Windows\system32> dir c:\tools

Directory: C:\tools

Mode LastWriteTime Length Name

d----- 6/2/2016 5:02 PM src

PS C:\Windows\system32> fsutil reparsepoint query c:\tools
Reparse Tag Value : 0xa000000c
Tag value: Microsoft
Tag value: Name Surrogate
Tag value: Symbolic Link

Reparse Data Length: 0x00000116
Reparse Data:
0000: 00 00 80 00 82 00 86 00 00 00 00 00 5c 00 43 00 .............C.
0010: 6f 00 6e 00 74 00 61 00 69 00 6e 00 65 00 72 00 o.n.t.a.i.n.e.r.
0020: 4d 00 61 00 70 00 70 00 65 00 64 00 44 00 69 00 M.a.p.p.e.d.D.i.
0030: 72 00 65 00 63 00 74 00 6f 00 72 00 69 00 65 00 r.e.c.t.o.r.i.e.
0040: 73 00 5c 00 45 00 36 00 44 00 35 00 46 00 45 00 s..E.6.D.5.F.E.
0050: 44 00 46 00 2d 00 32 00 46 00 37 00 30 00 2d 00 D.F.-.2.F.7.0.-.
0060: 34 00 42 00 35 00 43 00 2d 00 41 00 31 00 43 00 4.B.5.C.-.A.1.C.
0070: 37 00 2d 00 33 00 37 00 44 00 33 00 30 00 46 00 7.-.3.7.D.3.0.F.
0080: 38 00 45 00 38 00 45 00 43 00 45 00 00 00 5c 00 8.E.8.E.C.E....
0090: 5c 00 3f 00 5c 00 43 00 6f 00 6e 00 74 00 61 00 .?..C.o.n.t.a.
00a0: 69 00 6e 00 65 00 72 00 4d 00 61 00 70 00 70 00 i.n.e.r.M.a.p.p.
00b0: 65 00 64 00 44 00 69 00 72 00 65 00 63 00 74 00 e.d.D.i.r.e.c.t.
00c0: 6f 00 72 00 69 00 65 00 73 00 5c 00 45 00 36 00 o.r.i.e.s..E.6.
00d0: 44 00 35 00 46 00 45 00 44 00 46 00 2d 00 32 00 D.5.F.E.D.F.-.2.
00e0: 46 00 37 00 30 00 2d 00 34 00 42 00 35 00 43 00 F.7.0.-.4.B.5.C.
00f0: 2d 00 41 00 31 00 43 00 37 00 2d 00 33 00 37 00 -.A.1.C.7.-.3.7.
0100: 44 00 33 00 30 00 46 00 38 00 45 00 38 00 45 00 D.3.0.F.8.E.8.E.
0110: 43 00 45 00 00 00 C.E...
PS C:\Windows\system32>

Here is a different symbolic link on Windows 10 :

PS E:\temp> fsutil reparsepoint query t1
Reparse Tag Value : 0xa000000c
Tag value: Microsoft
Tag value: Name Surrogate
Tag value: Symbolic Link

Reparse Data Length: 0x00000060
Reparse Data:
0000: 2a 00 2a 00 00 00 2a 00 00 00 00 00 5c 00 5c 00 ....*.......
0010: 3f 00 5c 00 65 00 3a 00 5c 00 74 00 65 00 6d 00 ?..e.:..t.e.m.
0020: 70 00 5c 00 73 00 6f 00 6d 00 65 00 77 00 68 00 p..s.o.m.e.w.h.
0030: 65 00 72 00 65 00 5c 00 3f 00 3f 00 5c 00 65 00 e.r.e..?.?..e.
0040: 3a 00 5c 00 74 00 65 00 6d 00 70 00 5c 00 73 00 :..t.e.m.p..s.
0050: 6f 00 6d 00 65 00 77 00 68 00 65 00 72 00 65 00 o.m.e.w.h.e.r.e.

The Problem is demonstrated in the first 16 bytes from each Operating System :
WS 2016 : 0000: 00 00 80 00 82 00 86 00 00 00 00 00 5c 00 43 00 .............C.
Windows 10: 0000: 2a 00 2a 00 00 00 2a 00 00 00 00 00 5c 00 5c 00 ....*.......

What has happened is that the 'PrintName' and 'SubstituteName' in the dynamic portion of the result have swapped positions, so now PrintNameOffset is non-zero on Windows Server 2016. This exposes the substraction result producing a bogus slice. My guess is in all other operating systems, (i.e. Windows 10) the PrintName field was placed first, meaning that PrintNameOffset was always zero, concealing the bug.

@nkaethler if you know what to fix, here https://golang.org/doc/contribute.html is how to contribute. New test that demonstrates the problem would be nice. Even if you have a test that only works on some OS versions.

Alex

Any update on this? Do we have a standalone test that shows the problem?

Any update on this?

I am looking into it. Hopefully I will have something by the end of today.

Alex

I agree with @nkaethler that we should change:

s = UTF16ToString(p[data.PrintNameOffset/2 : (data.PrintNameLength - data.PrintNameOffset)/2])

for

s = UTF16ToString(p[data.PrintNameOffset/2 : (data.PrintNameLength + data.PrintNameOffset)/2])

Current code works because everything we test has data.PrintNameOffset set to 0 (and it does not matter if we use + or -). But "Junction.exe published by sysinternals", unlike standard mklink command, sets data.PrintNameLength to 0. I have found some explanation here https://svn.boost.org/trac/boost/ticket/10900
read_symlink extracts the reparse point's PrintName?, which isn't guaranteed to be valid - according to http://msdn.microsoft.com/en-us/library/cc232006.aspx, "the print name SHOULD be an informative pathname, suitable for display to a user, that also identifies the target of the symbolic link", but it's not guaranteed to be the actual target (most notably, with junctions created using the Sysinternals junction.exe tool, the PrintName? is completely empty). It should probably be extracting the SubstituteName? instead, though that would likely require stripping off the leading "\??\" to yield a properly usable pathname (unless it's a device ID, in which case all bets are off).
I also think we should use SubstituteName? instead of PrintName?, just like the article recommends. But SubstituteName? needs to be "cleaned up" first. I don't think we should change to SubstituteName? for go1.7.

Also I don't have a test for either change.

Alex

I still cannot reproduce this crash. I tried different ways to create both symbolic links and directory junctions to make Go crash, but I cannot. I have found a new issue (#16145) along the way, but I cannot recreate this issue.

Looking at output above:

PS C:\Windows\system32> fsutil reparsepoint query c:\tools
Reparse Tag Value : 0xa000000c
Tag value: Microsoft
Tag value: Name Surrogate
Tag value: Symbolic Link

Reparse Data Length: 0x00000116
Reparse Data:
0000: 00 00 80 00 82 00 86 00 00 00 00 00 5c 00 43 00 .............C.
0010: 6f 00 6e 00 74 00 61 00 69 00 6e 00 65 00 72 00 o.n.t.a.i.n.e.r.
0020: 4d 00 61 00 70 00 70 00 65 00 64 00 44 00 69 00 M.a.p.p.e.d.D.i.
0030: 72 00 65 00 63 00 74 00 6f 00 72 00 69 00 65 00 r.e.c.t.o.r.i.e.
0040: 73 00 5c 00 45 00 36 00 44 00 35 00 46 00 45 00 s..E.6.D.5.F.E.
0050: 44 00 46 00 2d 00 32 00 46 00 37 00 30 00 2d 00 D.F.-.2.F.7.0.-.
0060: 34 00 42 00 35 00 43 00 2d 00 41 00 31 00 43 00 4.B.5.C.-.A.1.C.
0070: 37 00 2d 00 33 00 37 00 44 00 33 00 30 00 46 00 7.-.3.7.D.3.0.F.
0080: 38 00 45 00 38 00 45 00 43 00 45 00 00 00 5c 00 8.E.8.E.C.E....
0090: 5c 00 3f 00 5c 00 43 00 6f 00 6e 00 74 00 61 00 .?..C.o.n.t.a.
00a0: 69 00 6e 00 65 00 72 00 4d 00 61 00 70 00 70 00 i.n.e.r.M.a.p.p.
00b0: 65 00 64 00 44 00 69 00 72 00 65 00 63 00 74 00 e.d.D.i.r.e.c.t.
00c0: 6f 00 72 00 69 00 65 00 73 00 5c 00 45 00 36 00 o.r.i.e.s..E.6.
00d0: 44 00 35 00 46 00 45 00 44 00 46 00 2d 00 32 00 D.5.F.E.D.F.-.2.
00e0: 46 00 37 00 30 00 2d 00 34 00 42 00 35 00 43 00 F.7.0.-.4.B.5.C.
00f0: 2d 00 41 00 31 00 43 00 37 00 2d 00 33 00 37 00 -.A.1.C.7.-.3.7.
0100: 44 00 33 00 30 00 46 00 38 00 45 00 38 00 45 00 D.3.0.F.8.E.8.E.
0110: 43 00 45 00 00 00 C.E...
PS C:\Windows\system32>

this is symbolic link with:

ReparseDataLength=0x116 Reserved=0x0
SubstituteNameOffset=0x0 SubstituteNameLength=0x80
PrintNameOffset=0x82 PrintNameLength=0x86
Flags=0x0
PathBuffer=
00000000  5c 00 43 00 6f 00 6e 00  74 00 61 00 69 00 6e 00  |\.C.o.n.t.a.i.n.|
00000010  65 00 72 00 4d 00 61 00  70 00 70 00 65 00 64 00  |e.r.M.a.p.p.e.d.|
00000020  44 00 69 00 72 00 65 00  63 00 74 00 6f 00 72 00  |D.i.r.e.c.t.o.r.|
00000030  69 00 65 00 73 00 5c 00  45 00 36 00 44 00 35 00  |i.e.s.\.E.6.D.5.|
00000040  46 00 45 00 44 00 46 00  2d 00 32 00 46 00 37 00  |F.E.D.F.-.2.F.7.|
00000050  30 00 2d 00 34 00 42 00  35 00 43 00 2d 00 41 00  |0.-.4.B.5.C.-.A.|
00000060  31 00 43 00 37 00 2d 00  33 00 37 00 44 00 33 00  |1.C.7.-.3.7.D.3.|
00000070  30 00 46 00 38 00 45 00  38 00 45 00 43 00 45 00  |0.F.8.E.8.E.C.E.|
00000080  00 00 5c 00 5c 00 3f 00  5c 00 43 00 6f 00 6e 00  |..\.\.?.\.C.o.n.|
00000090  74 00 61 00 69 00 6e 00  65 00 72 00 4d 00 61 00  |t.a.i.n.e.r.M.a.|
000000a0  70 00 70 00 65 00 64 00  44 00 69 00 72 00 65 00  |p.p.e.d.D.i.r.e.|
000000b0  63 00 74 00 6f 00 72 00  69 00 65 00 73 00 5c 00  |c.t.o.r.i.e.s.\.|
000000c0  45 00 36 00 44 00 35 00  46 00 45 00 44 00 46 00  |E.6.D.5.F.E.D.F.|
000000d0  2d 00 32 00 46 00 37 00  30 00 2d 00 34 00 42 00  |-.2.F.7.0.-.4.B.|
000000e0  35 00 43 00 2d 00 41 00  31 00 43 00 37 00 2d 00  |5.C.-.A.1.C.7.-.|
000000f0  33 00 37 00 44 00 33 00  30 00 46 00 38 00 45 00  |3.7.D.3.0.F.8.E.|
00000100  38 00 45 00 43 00 45 00  00 00 86 bc 32 12 1e 01  |8.E.C.E.....2...|
00000110  00 00 1e 01 00 00 ef ac  48 00 00 00 00 00 00 00  |........H.......|
00000120  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000130  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
SubstituteName="\\ContainerMappedDirectories\\E6D5FEDF-2F70-4B5C-A1C7-37D30F8E8ECE"
PrintName="\\\\?\\ContainerMappedDirectories\\E6D5FEDF-2F70-4B5C-A1C7-37D30F8E8ECE"

I can see how link like that can crash Go, but I cannot re-create link like that. @nkaethler if you could tell us how you created the link, that would be helpful.

If we don't have a test, I still think we should change syscall.ReadLink as I commented above for go1.7. I don't see how it could be worse than it already is. Here https://go-review.googlesource.com/24330 is the CL. Please, review it, if you think it is worth going forward with.

Alex

I still cannot reproduce this crash.

Here you are.

package os_test

import (
    "encoding/binary"
    "io/ioutil"
    "os"
    "syscall"

    "testing"
)

const FSCTL_SET_REPARSE_POINT = 0x900a4

var le = binary.LittleEndian

func TestReadlink(t *testing.T) {
    defer chtmpdir(t)()

    f, err := os.Create("test")
    if err != nil {
        t.Fatal(err)
    }
    f.Close()

    err = Symlink("test", "linkToTest")
    if err != nil {
        t.Fatal(err)
    }

    defer func() {
        if r := recover(); r != nil {
            t.Fatal(r)
        }
    }()

    target, err := os.Readlink("linkToTest")
    if err != nil {
        t.Fatal(err)
    }

    if target != "test" {
        t.Error("readlink is broken")
    }
}

func Symlink(symlinkfilename string, targetfilename string) (err error) {
    // typedef struct _REPARSE_DATA_BUFFER {
    //   uint32 ReparseTag;
    //   uint16 ReparseDataLength;
    //   uint16 Reserved;
    //   uint16 SubstituteNameOffset;
    //   uint16 SubstituteNameLength;
    //   uint16 PrintNameOffset;
    //   uint16 PrintNameLength;
    //   uint32  Flags;
    //   WCHAR  PathBuffer[1];
    // } REPARSE_DATA_BUFFER, *PREPARSE_DATA_BUFFER;

    fd, err := syscall.CreateFile(syscall.StringToUTF16Ptr(targetfilename), syscall.GENERIC_READ|syscall.GENERIC_WRITE, 0, nil, syscall.CREATE_ALWAYS,
        syscall.FILE_FLAG_OPEN_REPARSE_POINT|syscall.FILE_FLAG_BACKUP_SEMANTICS, 0)
    if err != nil {
        return err
    }
    defer syscall.CloseHandle(fd)

    sympath := syscall.StringToUTF16(symlinkfilename)

    sympath = sympath[:len(sympath)-1]

    rdbbuf := make([]byte, 20+len(sympath)*4)

    le.PutUint32(rdbbuf[:4], syscall.IO_REPARSE_TAG_SYMLINK)
    le.PutUint16(rdbbuf[4:6], uint16(len(rdbbuf)-8))
    le.PutUint16(rdbbuf[8:10], 0)
    le.PutUint16(rdbbuf[10:12], uint16(len(sympath)*2))
    for i, w := range sympath {
        le.PutUint16(rdbbuf[20+i*2:20+i*2+2], w)
    }
    le.PutUint16(rdbbuf[12:14], uint16(len(sympath)*2))
    le.PutUint16(rdbbuf[14:16], uint16(len(sympath)*2))
    for i, w := range sympath {
        le.PutUint16(rdbbuf[20+len(sympath)*2+i*2:20+len(sympath)*2+i*2+2], w)
    }

    return syscall.DeviceIoControl(fd, FSCTL_SET_REPARSE_POINT, &rdbbuf[0], uint32(len(rdbbuf)), nil, 0, nil, nil)
}

// chtmpdir changes the working directory to a new temporary directory and
// provides a cleanup function. Used when PWD is read-only.
func chtmpdir(t *testing.T) func() {
    oldwd, err := os.Getwd()
    if err != nil {
        t.Fatalf("chtmpdir: %v", err)
    }
    d, err := ioutil.TempDir("", "test")
    if err != nil {
        t.Fatalf("chtmpdir: %v", err)
    }
    if err := os.Chdir(d); err != nil {
        t.Fatalf("chtmpdir: %v", err)
    }
    return func() {
        if err := os.Chdir(oldwd); err != nil {
            t.Fatalf("chtmpdir: %v", err)
        }
        os.RemoveAll(d)
    }
}

@hirochachacha, thank you for suggestion. I tried it:

diff --git a/src/os/os_windows_test.go b/src/os/os_windows_test.go
index 05d7a8f..dae4c04 100644
--- a/src/os/os_windows_test.go
+++ b/src/os/os_windows_test.go
@@ -5,6 +5,7 @@
 package os_test

 import (
+   "encoding/binary"
    "io/ioutil"
    "os"
    osexec "os/exec"
@@ -245,3 +246,66 @@ func TestDeleteReadOnly(t *testing.T) {
        t.Fatal(err)
    }
 }
+
+func createSymlink(symlinkfilename string, targetfilename string) (err error) {
+   const FSCTL_SET_REPARSE_POINT = 0x900a4
+   var le = binary.LittleEndian
+   // typedef struct _REPARSE_DATA_BUFFER {
+   //   uint32 ReparseTag;
+   //   uint16 ReparseDataLength;
+   //   uint16 Reserved;
+   //   uint16 SubstituteNameOffset;
+   //   uint16 SubstituteNameLength;
+   //   uint16 PrintNameOffset;
+   //   uint16 PrintNameLength;
+   //   uint32  Flags;
+   //   WCHAR  PathBuffer[1];
+   // } REPARSE_DATA_BUFFER, *PREPARSE_DATA_BUFFER;
+   fd, err := syscall.CreateFile(syscall.StringToUTF16Ptr(targetfilename), syscall.GENERIC_READ|syscall.GENERIC_WRITE, 0, nil, syscall.CREATE_ALWAYS,
+       syscall.FILE_FLAG_OPEN_REPARSE_POINT|syscall.FILE_FLAG_BACKUP_SEMANTICS, 0)
+   if err != nil {
+       return err
+   }
+   defer syscall.CloseHandle(fd)
+   sympath := syscall.StringToUTF16(symlinkfilename)
+   sympath = sympath[:len(sympath)-1]
+   rdbbuf := make([]byte, 20+len(sympath)*4)
+   le.PutUint32(rdbbuf[:4], syscall.IO_REPARSE_TAG_SYMLINK)
+   le.PutUint16(rdbbuf[4:6], uint16(len(rdbbuf)-8))
+   le.PutUint16(rdbbuf[8:10], 0)
+   le.PutUint16(rdbbuf[10:12], uint16(len(sympath)*2))
+   for i, w := range sympath {
+       le.PutUint16(rdbbuf[20+i*2:20+i*2+2], w)
+   }
+   le.PutUint16(rdbbuf[12:14], uint16(len(sympath)*2))
+   le.PutUint16(rdbbuf[14:16], uint16(len(sympath)*2))
+   for i, w := range sympath {
+       le.PutUint16(rdbbuf[20+len(sympath)*2+i*2:20+len(sympath)*2+i*2+2], w)
+   }
+   return syscall.DeviceIoControl(fd, FSCTL_SET_REPARSE_POINT, &rdbbuf[0], uint32(len(rdbbuf)), nil, 0, nil, nil)
+}
+
+func TestReadlink(t *testing.T) {
+   defer chtmpdir(t)()
+   f, err := os.Create("test")
+   if err != nil {
+       t.Fatal(err)
+   }
+   f.Close()
+   err = createSymlink("test", "linkToTest")
+   if err != nil {
+       t.Fatal(err)
+   }
+   defer func() {
+       if r := recover(); r != nil {
+           t.Fatal(r)
+       }
+   }()
+   target, err := os.Readlink("linkToTest")
+   if err != nil {
+       t.Fatal(err)
+   }
+   if target != "test" {
+       t.Error("readlink is broken")
+   }
+}

but the test fails on both my Windows XP:

C:\Documents and Settings\brainman>u:\test -test.v -test.run=Readlink
=== RUN   TestReadlink
Exception 0xc0000005 0x1 0x0 0x7c80168e
PC=0x7c80168e

syscall.Syscall9(0x7c801629, 0x8, 0x780, 0x900a4, 0x10d862d0, 0x24, 0x0, 0x0, 0x0, 0x0, ...)
        /root/go/src/runtime/syscall_windows.go:185 +0x4a
syscall.DeviceIoControl(0x780, 0x900a4, 0x10d862d0, 0x24, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0)
        /root/go/src/syscall/zsyscall_windows.go:1314 +0xaf
os_test.createSymlink(0x544e44, 0x4, 0x545f55, 0xa, 0x0, 0x0)
        /root/go/src/os/os_windows_test.go:285 +0x6ff
os_test.TestReadlink(0x10d80280)
        /root/go/src/os/os_windows_test.go:295 +0x131
testing.tRunner(0x10d80280, 0x55d68c)
        /root/go/src/testing/testing.go:610 +0x8c
created by testing.(*T).Run
        /root/go/src/testing/testing.go:646 +0x2a5

goroutine 1 [chan receive]:
testing.(*T).Run(0x10d80280, 0x5466e5, 0xc, 0x55d68c, 0x1d1cc01)
        /root/go/src/testing/testing.go:647 +0x2c4
testing.RunTests.func1(0x10d80200)
        /root/go/src/testing/testing.go:793 +0xa1
testing.tRunner(0x10d80200, 0x10dafed0)
        /root/go/src/testing/testing.go:610 +0x8c
testing.RunTests(0x55d56c, 0x5d1540, 0x44, 0x44, 0x10db6201)
        /root/go/src/testing/testing.go:799 +0x332
testing.(*M).Run(0x10daff74, 0x10db0180)
        /root/go/src/testing/testing.go:743 +0x79
main.main()
        os/_test/_testmain.go:194 +0x100
eax     0x0
ebx     0x0
ecx     0x0
edx     0x7c90e514
edi     0x6feec
esi     0x0
ebp     0x6fec4
esp     0x6fe94
eip     0x7c80168e
eflags  0x10246
cs      0x1b
fs      0x3b
gs      0x0

and my Windows 7:

C:\Users\brainman>u:\test -test.v -test.run=Readlink
=== RUN   TestReadlink
--- FAIL: TestReadlink (0.02s)
        os_windows_test.go:297: Access is denied.
FAIL

What do I do wrong?

Alex

What do I do wrong?

Did you try it as administrator?

Did you try it as administrator?

How do I do that?

Alex

How do I do that?

I mean use Run as administrator to invoke your terminal.

Btw, I tested it as a single file - readlink_test.go with go test readlink_test.go command.

administrator:

readlink_test.go:32: runtime error: slice bounds out of range

normal user:

readlink_test.go:27: Access is denied.

My machine is windows 10.

I mean use Run as administrator to invoke your terminal.

It does not work. I get same result:

=== RUN   TestReadlink
--- FAIL: TestReadlink (0.01s)
        readlink_test.go:24: Access is denied.
FAIL

Alex

 func TestReadlink(t *testing.T) {
+  if !supportsSymlinks {
+    t.Skip()
+  }

You mean this patch doesn't affect the result, right?
If so, only suspicious part for me is syscall.CreateFile.
How about using MAXIMUM_ALLOWED = 0x02000000 and syscall.FILE_ATTRIBUTE_REPARSEPOINT?

https://msdn.microsoft.com/ja-jp/library/windows/desktop/aa374892(v=vs.85).aspx
https://msdn.microsoft.com/en-us/library/windows/desktop/gg258117(v=vs.85).aspx

I cannot test on windows XP and 7 myself, sorry.

You mean this patch doesn't affect the result, right?

It skips new test on Windows XP, but has no effect on Windows 7.

How about using MAXIMUM_ALLOWED = 0x02000000 and syscall.FILE_ATTRIBUTE_REPARSEPOINT?

Replacing

syscall.FILE_FLAG_OPEN_REPARSE_POINT|syscall.FILE_FLAG_BACKUP_SEMANTICS

with

syscall.FILE_FLAG_OPEN_REPARSE_POINT|syscall.FILE_FLAG_BACKUP_SEMANTICS|FILE_ATTRIBUTE_REPARSEPOINT|MAXIMUM_ALLOWED

does not help.

Alex

Hmm, how about this?

le.PutUint32(rdbbuf[16:20], 0x00000001) // SYMLINK_FLAG_RELATIVE

Hmm, how about this?

I tested it on windows 7 VM downloaded from microsoft.
But nothing changed.

Finally, it works on my windows 10 and windows 7.
https://gist.github.com/hirochachacha/f5b20c319790ee02b64bd3a9a94bdf5b

Finally, it works on my windows 10 and windows 7.
https://gist.github.com/hirochachacha/f5b20c319790ee02b64bd3a9a94bdf5b

Yes that works for me too on Windows 7. Thank you very much.

But I still get

C:\Documents and Settings\brainman>u:\test -test.v -test.run=Readlink
=== RUN   TestReadlink
--- FAIL: TestReadlink (0.02s)
        readlink_test.go:35: A specified privilege does not exist.
FAIL

on Windows XP.

I am also worried about you using a lot of unsafe code (building Windows structs by writing into binary.LittleEndian). This could introduce memory corruption in "os" test binary.

I would leave test for go1.8. But feel free to send a CL yourself - if others want to proceed with it, I will help along too.

Thanks again. I will stop thinking about this bug now. :-)

Alex

on Windows XP.

As you know, Windows XP doesn't support symlink, I guess it's also impossible to create symlink.
So, I think we should skip the test by supportsSymlinks variable, practically.

I am also worried about you using a lot of unsafe code (building Windows structs by writing into binary.LittleEndian). This could introduce memory corruption in "os" test binary.

We can avoid binary.LittleEndian by unsafe hack (e.g *(*uint32)(unsafe.Pointer(&bs[0])) = uint32(10), or cast to some struct)
on windows PC, that is what we done in syscall package. But yes, unsafe is unsafe...

I would leave test for go1.8. But feel free to send a CL yourself - if others want to proceed with it, I will help along too.

I see. It's bigger test than I expected, which mean, it contains unknown bugs, potentially.

Thanks again. I will stop thinking about this bug now. :-)

Thank you for your confirmations. :)

@hirochachacha please check https://go-review.googlesource.com/#/c/25320/ for the test. We cannot submit the changes until go1.7 is released, but we can review code now. Thank you.

Alex

I think we should share correct behavior of the function at first.
Here is mklink's behavior on windows 10. ( I hope it is applicable to other versions)

So, I think CreateSymbolicLink should behave like this:

func CreateSymbolicLink(link, target string) {
  ...
  // build reparse data buffer
  volume := filepath.VolumeName(target)
  switch len(volume) {
  case 0:
    // set PrintName to target
    // set SubstituteName to target
    // set Flags to 1
  case 2:
    target = filepath.Abs(target)
    // set PrintName to target
    // set SubstituteName to "\??\" + target
  default: // isUNC
    // set PrintName to target
    // set SubstituteName to "\??\UNC" + target[1:]
  }
  ...
}

And,

func CreateMountPoint(link, target string) {
  ...
  // build reparse data buffer
  target = filepath.Abs(target)
  // set PrintName to target
  // set SubstituteName to "\??\" + target
  ...
}

@alexbrainman Btw, can you make a symlink on windows XP if you skip SeCreateSymbolicLinkPrivilege?
If yes, we may be possible to set malformed data (e.g PrintNameOffset beyond buffer size) by DeviceIoControl, that can be useful to input validation tests.

Ah.. why do we need to test our own implementation of symlink in the first place?
Our issues are just Readlink related.

I think we should share correct behavior of the function at first.

Please do not comment my code here. Do it on Gerrit https://go-review.googlesource.com/#/c/25320/ Otherwise I will lose your comments. Thank you.

can you make a symlink on windows XP if you skip SeCreateSymbolicLinkPrivilege?

Windows XP does not have CreateSymbolicLink.

why do we need to test our own implementation of symlink in the first place?
Our issues are just Readlink related.

I don't understand your question. Please try again.

Alex

Windows XP does not have CreateSymbolicLink.

Yes. Can we emulate CreateSymbolicLink by DeviceIoControl?
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365511(v=vs.85).aspx says,
reparse point supports user-defined data.

I don't understand your question. Please try again.

I will comment on gerrit.

Can we emulate CreateSymbolicLink by DeviceIoControl?

That is what my CL 25320 is effectively doing already. Isn't it? I don't understand what more do you propose we do.

I will comment on gerrit.

I will answer there. Thank you.

Alex

That is what my CL 25320 is effectively doing already. Isn't it? I don't understand what more do you propose we do.

Yes it is except windows XP. Again, can we emulate CreateSymbolicLink on windows XP?

You said:

Yes that works for me too on Windows 7. Thank you very much.
But I still get
C:\Documents and Settings\brainman>u:\test -test.v -test.run=Readlink
=== RUN TestReadlink
--- FAIL: TestReadlink (0.02s)
readlink_test.go:35: A specified privilege does not exist.
FAIL
on Windows XP.

before.
If you skip privilege error, is it possible to make a symlink on windows XP?

Again, can we emulate CreateSymbolicLink on windows XP?

I was afraid you will say that. :-)
Do you know for certain how CreateSymbolicLink should be implemented on Windows XP? Microsoft didn't implement it - that makes it this idea even more questionable. Many parts of Windows XP, probably, do not expect "symbolic links" to exit. What will users do, if they OS will get confused?

I think it is bad idea to implement CreateSymbolicLink by hand. Even for testing, this is questionable. But at least we're doing it on OS versions that have CreateSymbolicLink implementation already. And we're doing it in a "controlled" environment - during our tests. And I don't see alternative to test our bugs.

Alex

I'm not talking about correct CreateSymbolicLink emulation.
I think Readlink should not panic by any input data.
IMPO, Readlink should validate input data itself without relying on OS's implicit behavior.
(because it's not on spec)
If window XP doesn't reserve symbolic tag, there are possibility to make same structure as a malformed data. And if we transfer that file to windows 10 (I don't know it is possible or not),
we can propagate malformed data. Or there is a something easier way to make malformed input on windows 10. I just want an evidence for necessity of validation.

OS's implicit behavior

If invalid data is given to DeviceIoControl, windows 10 return ERROR_INVALID_REPARSE_DATA.
But it doesn't well explained. Maybe some versions are not.

FWIW, here are part of Readlink.

    rdbbuf := make([]byte, MAXIMUM_REPARSE_DATA_BUFFER_SIZE)
    var bytesReturned uint32
    err = DeviceIoControl(fd, FSCTL_GET_REPARSE_POINT, nil, 0, &rdbbuf[0], uint32(len(rdbbuf)), &bytesReturned, nil)
    if err != nil {
        return -1, err
    }

    rdb := (*reparseDataBuffer)(unsafe.Pointer(&rdbbuf[0]))

I'm worry about unsafe casting without validation.

I think Readlink should not panic by any input data.

I agree. But I don't understand the rest of your argument. Sorry.

I'm worry about unsafe casting without validation.

What validation do you propose we add?

Alex

@alexbrainman

If window XP doesn't reserve symbolic tag, there are possibility to make same structure as a malformed data. And if we transfer that file to windows 10 (I don't know it is possible or not),
we can propagate malformed data. Or there is a something easier way to make malformed input on windows 10. I just want an evidence for necessity of validation.

I've retried it.
Here, I call "an invalid symlink" as a reparse point its SubstituteNameOffset exceeds PathBuffer's size.
First, I tried BackupWrite to create an invalid symlink instead of using DeviceIoControl on Windows 10. But results are nothing changed. It can create a normal symlink, can't create an invalid symlink. So I gave up making an invalid symlink on Windows 10.
Second, I tried to create an invalid symlink on Windows XP, and it succeeded.
Third, I mounted the drive (which contains invalid symlinks) on Windows 10.
I called os.Readlink with an invalid symlink, and I saw ERROR_INVALID_REPARSE_DATA.

Summary:

• DeviceIoControl validate both input/output data. Therefore, syscall.Readlink doesn't panic even without validation. (If DeviceIoControl's validation covered all cases. As I said MSDN doesn't explain validation behaviors)
• On Windows XP, it can panics. (I know os.Readlink on XP isn't useful, though)

So, I failed to collect good evidences for implementing our own validation.

I still think we should validate it, and there are similar issues that their CLs are without testing.
#16475 #15653

I'll let you decide what to do.

What validation do you propose we add?

• ensure that input data size is greater or equal to 20 (header size)
• ensure that input data size is greater or equal to 8 + ReparseDataLength
• ensure that XXXOffset, XXXLength are even number.
• ensure that XXXOffset+XXXLength doesn't exceed PathBuffer's size

Thank you.

@alexbrainman

I am sorry, but I don't understand your point. Thank you for all that explanation, but I don't see what all this relates to. If it relates to my CL 25320, then comment my code there. If you are discussing solution to this current issue, then I didn't even think about that. I think we need some test first.

Alex

@nkaethler initially said:

Also note that

MAXIMUM_REPARSE_DATA_BUFFER_SIZE = 16 * 1024

So the following cast : (*[0xffff]uint16) seems to be risky, since that will exceed the size of the buffer that was allocated.

So I thought validation is part of this issue.

CL https://golang.org/cl/28010 mentions this issue.

CL https://golang.org/cl/25320 mentions this issue.

CL https://golang.org/cl/31118 mentions this issue.