syscall: panic within syscall_windows.go Readlink #15978
Comments
|
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).
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 ( func main() { } PS E:\temp> .\Junction\junction.exe goes_somewhere .\somewhere Junction v1.06 - Windows junction creator and reparse point viewer Created: E:\temp\goes_somewhere PS E:\temp> fsutil reparsepoint query .\goes_somewhere Reparse Data Length: 0x00000036 PS E:\temp> C:\users\nkaethler\s\p4\dev\src\Chroma\Go\bin\junction.exe -d goes_somewhere PS E:\temp> dir Mode LastWriteTime Length Name d----- 6/7/2016 11:17 AM a PS E:\temp> |
|
Here is a sample of what fsutil reparsepoint query shows on Windows Server 2016 TP4 : Windows PowerShell PS C:\Windows\system32> dir c:\tools Mode LastWriteTime Length Name d----- 6/2/2016 5:02 PM src PS C:\Windows\system32> fsutil reparsepoint query c:\tools Reparse Data Length: 0x00000116 Here is a different symbolic link on Windows 10 : PS E:\temp> fsutil reparsepoint query t1 Reparse Data Length: 0x00000060 The Problem is demonstrated in the first 16 bytes from each Operating System : 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? |
I am looking into it. Hopefully I will have something by the end of today. Alex |
|
I agree with @nkaethler that we should change: for 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 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: this is symbolic link with: 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 |
Here you are. |
|
@hirochachacha, thank you for suggestion. I tried it: but the test fails on both my Windows XP: and my Windows 7: What do I do wrong? Alex |
Did you try it as administrator? |
How do I do that? Alex |
I mean use |
|
Btw, I tested it as a single file - administrator: normal user: My machine is windows 10. |
It does not work. I get same result: Alex |
You mean this patch doesn't affect the result, right? https://msdn.microsoft.com/ja-jp/library/windows/desktop/aa374892(v=vs.85).aspx I cannot test on windows XP and 7 myself, sorry. |
It skips new test on Windows XP, but has no effect on Windows 7.
Replacing with does not help. Alex |
|
Hmm, how about this? |
I tested it on windows 7 VM downloaded from microsoft. |
|
Finally, it works on my windows 10 and windows 7. |
Yes that works for me too on Windows 7. Thank you very much. But I still get 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 |
As you know, Windows XP doesn't support symlink, I guess it's also impossible to create symlink.
We can avoid
I see. It's bigger test than I expected, which mean, it contains unknown bugs, potentially.
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.
So, I think 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 |
|
Ah.. why do we need to test our own implementation of |
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.
Windows XP does not have CreateSymbolicLink.
I don't understand your question. Please try again. Alex |
Yes. Can we emulate
I will comment on gerrit. |
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 answer there. Thank you. Alex |
Yes it is except windows XP. Again, can we emulate You said:
before. |
I was afraid you will say that. :-) 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 |
If invalid data is given to |
|
FWIW, here are part of 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 agree. But I don't understand the rest of your argument. Sorry.
What validation do you propose we add? Alex |
I've retried it. Summary:
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. I'll let you decide what to do.
Thank you. |
|
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:
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. |
Updates #15978 Updates #16145 Change-Id: I161f5bc97d41c08bf5e1405ccafa86232d70886d Reviewed-on: https://go-review.googlesource.com/25320 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Russ Cox <rsc@golang.org>
|
CL https://golang.org/cl/31118 mentions this issue. |
Please answer these questions before submitting your issue. Thanks!
go version)?go version go1.6.2 windows/amd64
go env)?Windows Server 2016 TP5, amd64
If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
Built a version of go-swagger, and run it Panic's when whilst compiling a swagger.json.
I believe the issue is in the following switch statement within syscall_windows.go Readlink() :
switch rdb.ReparseTag {
case IO_REPARSE_TAG_SYMLINK:
data := (symbolicLinkReparseBuffer)(unsafe.Pointer(&rdb.reparseBuffer))
p := ([0xffff]uint16)(unsafe.Pointer(&data.PathBuffer[0]))
s = UTF16ToString(p[data.PrintNameOffset/2 : (data.PrintNameLength-data.PrintNameOffset)/2])
case _IO_REPARSE_TAG_MOUNT_POINT:
data := (mountPointReparseBuffer)(unsafe.Pointer(&rdb.reparseBuffer))
p := ([0xffff]uint16)(unsafe.Pointer(&data.PathBuffer[0]))
s = UTF16ToString(p[data.PrintNameOffset/2 : (data.PrintNameLength-data.PrintNameOffset)/2])
Per Microsoft documentation at https://msdn.microsoft.com/en-us/library/ff552012.aspx
The PrintNameLength field should be interpreted as a length field, so the slice should change the subtraction to addition, as in :
s = UTF16ToString(p[data.PrintNameOffset/2 : (data.PrintNameLength + data.PrintNameOffset)/2])
I have no idea why I may be getting a non-zero PrintNameOffset result, but if data.PrintNameOffset > data.PrintNameLength then it definitely seems to be possible to run into problems.
Also note that
So the following cast : (*[0xffff]uint16) seems to be risky, since that will exceed the size of the buffer that was allocated.
Here is a portion of the call stack:
goroutine 27 [running]:
panic(0xb20740, 0xc08202c010)
c:/go/src/runtime/panic.go:481 +0x3f4
syscall.Readlink(0xc0829632f0, 0x8, 0xc08213eb00, 0x80, 0x80, 0x0, 0x0, 0x0)
c:/go/src/syscall/syscall_windows.go:1031 +0x4e9
os.Readlink(0xc0829632f0, 0x8, 0x0, 0x0, 0x0, 0x0)
c:/go/src/os/file_posix.go:21 +0xc4
path/filepath.walkLink(0xc0829632f0, 0x8, 0xc0827dbb20, 0x0, 0x0, 0x0, 0x0, 0x0)
c:/go/src/path/filepath/symlink.go:47 +0x20c
path/filepath.walkLinks(0xc0821181e0, 0x8, 0xc0827dbb20, 0x0, 0x0
, 0x0, 0x0)
c:/go/src/path/filepath/symlink.go:77 +0x422
path/filepath.walkLinks(0xc0821181e0, 0x9, 0xc0827dbb20, 0x0, 0x0, 0x0, 0x0)
c:/go/src/path/filepath/symlink.go:68 +0x219
path/filepath.walkLinks(0xc0821181e0, 0xb, 0xc0827dbb20, 0x0, 0x0, 0x0, 0x0)
c:/go/src/path/filepath/symlink.go:73 +0x2df
path/filepath.walkSymlinks(0xc0821181e0, 0xb, 0x0, 0x0, 0x0, 0x0)
c:/go/src/path/filepath/symlink.go:98 +0xae
path/filepath.evalSymlinks(0xc0821181e0, 0xb, 0x0, 0x0, 0x0, 0x0)
c:/go/src/path/filepath/symlink_windows.go:50 +0x5d
path/filepath.EvalSymlinks(0xc0821181e0, 0xb, 0x0, 0x0, 0x0, 0x0)
c:/go/src/path/filepath/path.go:227 +0x4a
go/build.(_Context).hasSubdir(0x1059340, 0xc0821181e0, 0xb, 0xbf8f20, 0x1, 0x0, 0x0, 0xc082963200)
c:/go/src/go/build/build.go:145 +0x102
go/build.(_Context).Import.func3(0xc0821181e0, 0xb, 0x0, 0x0)
c:/go/src/go/build/build.go:578 +0xb0
go/build.(*Context).Import(0x1059340, 0xc082594d21, 0x1c, 0xbf8f20, 0x1, 0x0, 0x5e0114, 0x0, 0x0)
c:/go/src/go/build/build.go:608 +0x6842
go/build.Import(0xc082594d21, 0x1c, 0xbf8f20, 0x1, 0x0, 0xc082594d20, 0x0, 0x0)
c:/go/src/go/build/build.go:1096 +0x72
github.com/go-swagger/go-swagger/vendor/golang.org/x/tools/imports.importPathToNameGoPath(0xc082594d21, 0x1c, 0xbf8f20, 0x1, 0x0, 0x0)
c:/users/nkaethler/s/p4/dev/src/chroma/go/src/github.com/go-swagger/go-swagger/vendor/golang.org/x/tools/imports/fix.go:168 +0x64
github.com/go-swagger/go-swagger/vendor/golang.org/x/tools/imports.fixImports.func1(0x17c5428, 0xc082954cf0, 0x0, 0x0)
c:/users/nkaethler/s/p4/dev/src/chroma/go/src/github.com/go-swagger/go-swagger/vendor/golang.org/x/tools/imports/fix.go:74 +0x29f
github.com/go-swagger/go-swagger/vendor/golang.org/x/tools/imports.visitFn.Visit(0xc0829554d0, 0x17c5428, 0xc082954cf0, 0x0, 0x0)
c:/users/nkaethler/s/p4/dev/src/chroma/go/src/github.com/go-swagger/go-swagger/vendor/golang.org/x/tools/imports/fix.go:409 +0x43
4. What did you expect to see?
The text was updated successfully, but these errors were encountered: