-
Notifications
You must be signed in to change notification settings - Fork 17.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
all: get standard library building with -d=checkptr #34972
Comments
This code in package reflect is currently failing in checkptrAlignment, and seems to violate the safety rules because of the conversion to Lines 2184 to 2192 in 8c6876e
(There's similar code for etype just below.) |
I'll cover the sync/atomic.hammer* tests fixup :) |
This failure looks genuinely suspicious to me:
|
Change https://golang.org/cl/201841 mentions this issue: |
Change https://golang.org/cl/201839 mentions this issue: |
Uploaded a couple CLs to improve -d=checkptr, and address some issues. CL 201841 enumerates all of the remaining std cmd test failures that I'm aware of, but haven't investigated yet. |
Hey @mdempsky in regards to the sync/atomic.HammerLoad* tests that are flagged, what they do is that for 8 goroutines, run pairwise increments of the high and low parts of an int* or *uint and panic if high != low. To save the value at the end of the function, we invoke StorePointer(addr, newValue) where newValue is a raw uint64 value such as checkptr tries to find the object associated with newValue but unfortunately that's a raw value that's not attached to any object and perhaps beyond Go's address space. Writing such code is a valid usecase and here is a minimal repro to test that edge case package main
import (
"fmt"
"sync/atomic"
"unsafe"
)
func main() {
val := uint64(820338753727)
addr := (*unsafe.Pointer)(unsafe.Pointer(&val))
v := uintptr(atomic.LoadPointer(addr))
atomic.StorePointer(addr, unsafe.Pointer(v+1+1<<32))
xs := uintptr(atomic.LoadPointer(addr))
fmt.Printf("xs: %#x\n", xs)
} $ go run -gcflags=all=-d=checkptr main.go
panic: (runtime.ptrArith) (0x10c09c0,0xc00000c080)
goroutine 1 [running]:
main.main()
/Users/emmanuelodeke/Desktop/openSrc/bugs/golang/34972/main.go:13 +0x91
exit status 2 Trying to recognize cases that do plain arithmetic is going to be a tricky one :) |
Change https://golang.org/cl/201877 mentions this issue: |
@odeke-em Thanks for looking into it.
Regardless of the merit of the use case, if the example violates package unsafe's safety rules, then it's not valid Go. Your repro case can be simplified to just:
This is failing because 0xc000000000 is an offset into the Go heap (at least where Go tries to allocate it by default on 64-bit OSes), and package unsafe's safety rules don't allow you to fabricate a pointer into the heap out of nothing. To fix the test, it should either:
|
I saw that one coming in #13656 (comment)!
|
If so, those tests are overspecified: the language spec does not make any guarantees about allocations or escapes. They should be updated to skip (or use different thresholds) when they are not using a specific compiler in a specific configuration. |
Caught with: go test -a -short -gcflags=all=-d=checkptr log/syslog and: grep -rE '\*\[([^2]|.{2,})\].*\)\(unsafe.Pointer' syscall Updates #34972 Change-Id: Iafd199b3a34beb7cc3e88484bf2fbae45183f951 Reviewed-on: https://go-review.googlesource.com/c/go/+/201877 Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Agreed, though I don't think currently there's a way for tests to check what compiler flags were used. (Keep in mind too that different compiler flags can be used for different packages.) |
@mdempsky on OSX, I got a lot of:
when testing Note This error does not happen on Linux. |
@cuonglm Thanks for the report. It looks like maybe |
Change https://golang.org/cl/202157 mentions this issue: |
I tried, but still fails with |
Ugh, right. Okay, in that case, we should probably just make |
Change https://golang.org/cl/202158 mentions this issue: |
Change https://golang.org/cl/202177 mentions this issue: |
Same as CL 201877 did for package syscall. Updates golang/go#34972 Change-Id: I3929841ab32378516edafb1f02a84b1bdcc77bbd Reviewed-on: https://go-review.googlesource.com/c/sys/+/202177 Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Change https://golang.org/cl/202580 mentions this issue: |
A common idiom for turning an unsafe.Pointer into a slice is to write: s := (*[Big]T)(ptr)[:n:m] This technically violates Go's unsafe pointer rules (rule #1 says T2 can't be bigger than T1), but it's fairly common and not too difficult to recognize, so might as well allow it for now so we can make progress on #34972. This should be revisited if #19367 is accepted. Updates #22218. Updates #34972. Change-Id: Id824e2461904e770910b6e728b4234041d2cc8bc Reviewed-on: https://go-review.googlesource.com/c/go/+/201839 Run-TryBot: Matthew Dempsky <mdempsky@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Follow the idiom for allowing -d=checkptr to recognize and verify correctness. Updates #22218. Updates #34972. Change-Id: Ib6001c6f0e6dc535a36bcfaa1ae48e29e0c737f8 Reviewed-on: https://go-review.googlesource.com/c/go/+/202580 Run-TryBot: Matthew Dempsky <mdempsky@google.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org>
Actually, Can we just remove |
WSASendto was added in These were the days where syscall package was free for all to do as we pleased.
I will try and report it back here.
You have to complain to Microsoft https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsasendto Even better. You should complain to people who designed Berkeley sockets in the first place.
We can do anything you want. But we are using the function ourselves in net package. And other people might be using it too. So we would have to replace it with something that call real WSASendTo anyway. Alex |
Thanks!
Yeah, it's not great. But WSASendto exposes a perfectly safe API by taking the Sockaddr interface. That's unsafe internally, but only the syscall package can implement its methods, so that unsafeness doesn't leak through the user API. Sendto on UNIX works the same way. I think the mistake was just exporting the unsafe raw
I may be totally missing something, but I don't see any calls to WSASendTo in net (or x/net). I do see WSASendto. In fact, I couldn't find any calls to WSASendTo in the GitHub corpus I have access to (though I'm really confused about how much coverage that has). |
Change https://golang.org/cl/220544 mentions this issue: |
SGTM
I did not explain myself properly. net package calls syscall.WSASendto and syscall.WSASendto calls syscall.WSASendTo. So net package calls syscall.WSASendTo indirectly. WSASendto Windows API is needed to implement net package. Whichever way we interface to it, but we must use WSASendto Windows API. Anyway I fixed net.TestPacketConn in https://go-review.googlesource.com/c/go/+/220544/ Unfortunately I discovered new broken test in net (I have no idea how I missed it before). This is the output after CL 220544 is applied
I will try and fix that too. Alex |
Change https://golang.org/cl/222457 mentions this issue: |
Change https://golang.org/cl/222855 mentions this issue: |
It is unclear whether unaligned reads should be allowed, or if they are even actually a good idea here. However, while we figure that out, we should un-break 'go test -race' for users of this package. Updates golang/go#37644 Updates golang/go#37298 Updates golang/go#37715 Updates golang/go#34972 Updates golang/go#35128 Change-Id: I088f5703023e4f05ee274a6753e925973f12ac1b Reviewed-on: https://go-review.googlesource.com/c/crypto/+/222855 Run-TryBot: Bryan C. Mills <bcmills@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Matthew Dempsky <mdempsky@google.com>
WSASendto converts unsafe.Pointer to *syscall.RawSockaddrAny. But that violates every rule of https://golang.org/pkg/unsafe/#Pointer Implement WSASendto by calling Windows WSASendTo API by calling syscall.Syscall9 directly. This allows us to comply with (4) Conversion of a Pointer to a uintptr when calling syscall.Syscall rule. After this change, this commands succeeds: go test -a -short -gcflags=all=-d=checkptr -run=TestPacketConn net Updates #34972 Change-Id: Ib9a810bedf9e05251b7d3c7f69e15bfbd177ac62 Reviewed-on: https://go-review.googlesource.com/c/go/+/220544 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
The problem was discovered while running go test -a -short -gcflags=all=-d=checkptr -run=TestUDPConnSpecificMethods net WSAMsg is type defined by Windows. And WSAMsg.Name could point to two different structures for IPv4 and IPV6 sockets. Currently WSAMsg.Name is declared as *syscall.RawSockaddrAny. But that violates (1) Conversion of a *T1 to Pointer to *T2. rule of https://golang.org/pkg/unsafe/#Pointer When we convert *syscall.RawSockaddrInet4 into *syscall.RawSockaddrAny, syscall.RawSockaddrInet4 and syscall.RawSockaddrAny do not share an equivalent memory layout. Same for *syscall.SockaddrInet6 into *syscall.RawSockaddrAny. This CL changes WSAMsg.Name type to *syscall.Pointer. syscall.Pointer length is 0, and that at least makes type checker happy. After this change I was able to run go test -a -short -gcflags=all=-d=checkptr std cmd without type checker complaining. Updates #34972 Change-Id: Ic5c2321c20abd805c687ee16ef6f643a2f8cd93f Reviewed-on: https://go-review.googlesource.com/c/go/+/222457 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Ian Lance Taylor <iant@golang.org>
I think I finished all -d=checkptr related changes. Running (on top of 801cd7c )
There are still 2 failures. But I don't think they are related to -d=checkptr. I am not sure what to do next. Should I send windows version of https://go-review.googlesource.com/c/go/+/201783/ ? Thank you. Alex |
Yay!
Do they also happen without checkptr? I agree they certainly don't look related to checkptr, but the dashboard is clean for Windows right now.
That would be awesome. |
Do those resolve if you run |
Change https://golang.org/cl/227003 mentions this issue: |
Yes. Running
fails in exactly the same way as
command.
I sent https://golang.org/cl/227003 But it fails here when I run all.bat with this error:
Mind you, I created CL on current tip. And current tip has new linker code. Maybe failure is related. I did not investigate it properly.
Running
before
makes things worse (I am showing just errors)
and
All tested on top of 801cd7c. Alex |
CL 201783 enable -d=checkptr when -race or -msan is specified everywhere but windows. But, now that all unsafe pointer conversions in the standard library are fixed, enable -d=checkptr even on windows. Updates #34964 Updates #34972 Change-Id: Id912fa83b0d5b46c6f1c134c742fd94d2d185835 Reviewed-on: https://go-review.googlesource.com/c/go/+/227003 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TestLogOpt failure is unrelated to CL 227003. I filled #38251 for that. Alex |
And I submitted CL 227003. So I don't see what else to do for this current issue. Leaving it for others to close. Alex |
Thanks @alexbrainman! |
Change https://golang.org/cl/228858 mentions this issue: |
Change https://golang.org/cl/225418 mentions this issue: |
CL 208617 introduced syscall.utf16PtrToString and internal/syscall/windows.UTF16PtrToString functions. Original version of CL 208617 did not include syscall.utf16PtrToString and internal/syscall/windows.UTF16PtrToString max parameter. The parameter was added by Brad at the request of Ian. Ian said: "In some cases it seems at least possible that the null terminator is not present. I think it would be safer if we passed a maximum length here." The syscall.utf16PtrToString and internal/syscall/windows.UTF16PtrToString function are designed to work with only null terminated strings. So max parameter is superfluous. This change removes max parameter. Updates #34972 Change-Id: Ifea65dbd86bca8a08353579c6b9636c6f963d165 Reviewed-on: https://go-review.googlesource.com/c/go/+/228858 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
This CL fixes unsafe casts to slices that are missing length or capacity. Running tests with -d=checkptr enabled may panic on casting unsafe.Pointer to a static array of large predefined length, that is most likely much bigger than the size of the actual array in memory. Checkptr check is not satisfied if slicing operator misses length and capacity arguments `(*[(1 << 30) - 1]uint16)(unsafe.Pointer(p))[:]`, or when there is no slicing at all `(*[(1 << 30) - 1]uint16)(unsafe.Pointer(p))`. To find all potential cases I used `grep -nr ")(unsafe.Pointer(" ./windows`, then filtered out safe casts when object size is always static and known at compile time. To reproduce the issue run tests with checkptr enabled `go test -a -gcflags=all=-d=checkptr ./windows/...`. Updates golang/go#34972 Fixes golang/go#38355 Change-Id: I9dd2084b4f9fb7618cdb140fb2f38b56b6d6cc04 GitHub-Last-Rev: 73288ad GitHub-Pull-Request: #65 Reviewed-on: https://go-review.googlesource.com/c/sys/+/225418 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
CL 208617 introduced syscall.utf16PtrToString and internal/syscall/windows.UTF16PtrToString functions. Original version of CL 208617 did not include syscall.utf16PtrToString and internal/syscall/windows.UTF16PtrToString max parameter. The parameter was added by Brad at the request of Ian. Ian said: "In some cases it seems at least possible that the null terminator is not present. I think it would be safer if we passed a maximum length here." The syscall.utf16PtrToString and internal/syscall/windows.UTF16PtrToString function are designed to work with only null terminated strings. So max parameter is superfluous. This change removes max parameter. Updates golang#34972 Change-Id: Ifea65dbd86bca8a08353579c6b9636c6f963d165 Reviewed-on: https://go-review.googlesource.com/c/go/+/228858 Run-TryBot: Alex Brainman <alex.brainman@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Bryan C. Mills <bcmills@google.com>
As a prerequisite for #34964, the standard library's tests need to all pass when
-d=checkptr
is enabled:Currently, that's not the case.
There are also some panics because of
(*[Big]T)(p)[:n]
expressions, like in package reflect. This code pattern should be recognized by cmd/compile.There are a few tests that fail because CL 201781 leads them to heap allocate when before things would only stack allocate; e.g., context.TestAllocs or database/sql.TestRawBytesAllocs. Not sure how to handle this; maybe for now the escape analysis change should only be enabled for
-d=checkptr=2
, and-race
/-msan
only enable-d=checkptr=1
.In sync/atomic.hammerStoreLoadPointer, there's (in effect):
This violates package unsafe's pointer rules: pointers have to be converted to uintptr and back to unsafe.Pointer in a single expression without being stored in a uintptr-typed variable, as is being done with
new
. (This just needs someone to grok exactly what the test is doing, and fix it to follow pointer rules correctly.)There seem to be other failures still that need to be diagnosed. If folks want to try running the command above, claiming a failure, and then investigating what's going on, that would be great.
The text was updated successfully, but these errors were encountered: