Skip to content
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

spec: write rules for safe use of unsafe.Pointer and uintptr #7192

Closed
ianlancetaylor opened this issue Jan 23, 2014 · 15 comments
Closed

spec: write rules for safe use of unsafe.Pointer and uintptr #7192

ianlancetaylor opened this issue Jan 23, 2014 · 15 comments

Comments

@ianlancetaylor
Copy link
Contributor

People are confused about when it is safe to convert an unsafe.Pointer to a uintptr.  If
the unsafe.Pointer is the only pointer to the object, when is the GC permitted to
collect it?  We need to write down some rules for constructs that are always safe.

It seems to me that we must permit
    unsafe.Pointer(uintptr(p) + offset)

It also seems to me that we must permit passing unsafe.Pointer values converted to
uintptr to syscall.Syscall.
    syscall.Syscall(SYS_OPEN, uintptr(unsafe.Pointer(StringBytePtr(n))))

If these sorts of constructs will not work safely, then we should consider disallowing
them.

And in general we must write down what is permitted.
@davecheney
Copy link
Contributor

Comment 1:

Thank you for raising this issue Ian.

@rsc
Copy link
Contributor

rsc commented May 27, 2014

Comment 3:

Labels changed: added release-go1.4, removed release-go1.3.

@mdempsky
Copy link
Member

mdempsky commented Aug 9, 2014

Comment 4:

Somewhat relatedly, the Go 1.3 release notes warn "Programs that use package unsafe to
store pointers in integer-typed values are also illegal but more difficult to diagnose
during execution."  Strictly interpreting this wording seems to imply even the
aforementioned "unsafe.Pointer(uintptr(p) + offset)" is illegal as the pointer value p
will be converted ("stored") to an integer-typed value during its evaluation.
Arguably saying "integer-typed variables" would be more appropriate (though see also
issue #8496 for issues with the term "variable").
Incidentally/alternatively, it might be nice if it was possible to allow some pointer
arithmetic directly on unsafe.Pointer values rather than always requiring them to be
converted to uintptr and back.  E.g., given "var p *int" and trying to get a pointer to
the byte that follows, it's quite tedious to need to write:
    q := (*byte)(unsafe.Pointer(uintptr(unsafe.Pointer(p)) + unsafe.Sizeof(*p)))
Whereas it would be a bit easier to read (IMO) as:
    q := (*byte)(unsafe.Pointer(p) + unsafe.Sizeof(*p))
and the GC safety implications would be clearer too I think.
As for syscall.Syscall(), one option might be to change "func Syscall(trap, a1, a2, a3
uintptr)" to "func Syscall(trap uintptr, a1, a2, a3 interface{})" and make callers
responsible for passing pointers instead of integers when they point to memory that
needs to be kept alive.
Unfortunately, that's not 100% Go1 compatible because (assuming 32-bit) the untyped
constant 1<<31 is assignable to uintptr but not interface{} (because the Go spec
says it should be converted to int first), so it might be necessary to instead provide a
new set of SyscallX() functions that can accept pointer values via interface{}
parameters and require users to switch.

@mdempsky
Copy link
Member

Comment 5:

I've put together a proposal for unsafe.Pointer arithmetic at
https://docs.google.com/document/d/1yyCMzE4YPfsXvnZNjhszaYNqavxHhvbY-OWPqdzZK30/pub.

@griesemer
Copy link
Contributor

Comment 6:

Here's a proposal based on the input from above: https://golang.org/cl/153110044

Owner changed to @griesemer.

Status changed to Started.

@gopherbot
Copy link

Comment 7:

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

@gopherbot
Copy link

Comment 8:

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

@griesemer
Copy link
Contributor

Comment 9:

This issue was closed by revision 5361b74.

Status changed to Fixed.

@alexbrainman
Copy link
Member

Comment 10:

> This issue was closed by revision 5db49b99612c.
> Status: Fixed 
I don't see how this fixes original issue - "write rules for safe use of unsafe.Pointer
and uintptr". The CL just adds: "... is implementation-defined". And that is fair
enough. But we still need to document somewhere what these rules are, even if they can
change in the future. We have plenty of code that uses unsafe.Pointer inside main repo -
who is to say if that code is correct. What about outside external packages? If you
don't have answers to this questions, that is OK too. But lets reopen the issue.
Thank you.
Alex

@griesemer
Copy link
Contributor

Comment 11:

Let's not re-open this issue. Here's a comment from rsc:
"go vet has a check for what I believe we should support long term. The Go 1.3 release
notes very nearly promised this (http://golang.org/doc/go1.3#garbage_collector). The
cases it allows are (given p unsafe.Pointer, v reflect.Value, h *reflect.SliceHeader or
*reflect.StringHeader):
unsafe.Pointer(uintptr(p) +/- delta))
unsafe.Pointer(uintptr(p))
unsafe.Pointer(v.Pointer())
unsafe.Pointer(v.UnsafeAddr())
unsafe.Pointer(h.Data)
The header variable h must be a pointer to an actual slice or string values; you cannot
just say 'var h reflect.SliceHeader' and use it."
The text in http://golang.org/doc/go1.3#garbage_collector has been updated and mentions
go vet as well. We may want to emphasize this again in the 1.4 release notes.
I'm not against exact rules, but the spec is probably not where they should be.
Giving exact rules also is not very helpful unless mechanically checked, hence go vet.
I've tried to propose a modified unsafe package that would have reduced the "dangerous"
operations to one, but it's not really making a big difference and still requires manual
inspection (besides the general opposition to changing the API).

@mdempsky
Copy link
Member

Comment 12:

As an aside, it seems worth pointing out that unsafe.Pointer(v.Pointer()) and
unsafe.Pointer(v.UnsafeAddr()) aren't 100% safe with gc, because if they're part of a
complex expression then gc spills them to uintptr-typed autotmp variables on the stack. 
E.g.,
    func foo(int) reflect.Value
    if unsafe.Pointer(foo(1).Pointer()) == unsafe.Pointer(foo(2).Pointer()) { ... }
is unsafe because gc compiles it as
    var autotmp1 uintptr = foo(1).Pointer()
    var autotmp2 uintptr = foo(2).Pointer()  // If foo(2) triggers a GC, autotmp1 might become invalid!
    if unsafe.Pointer(autotmp1) == unsafe.Pointer(autotmp2) { ... }
but "go vet" doesn't warn about this.

@alexbrainman
Copy link
Member

Comment 13:

Thank you for explanation.
> I'm not against exact rules, but the spec is probably not where they should be.
I have nothing against not putting them into spec. But they should be somewhere. I (as a
user) should be able to find them when I need them.
> ... hence go vet.
"go vet" failed me just the other day. I had this issue
https://code.google.com/p/odbc/issues/detail?id=51, which, I believe, is gc related. But
"go vet" didn't complained about that:
C:\go\path\src\code.google.com\p\odbc>hg par
changeset:   37:78314be168c8
user:        Alex Brainman <alex.brainman@gmail.com>
date:        Tue Sep 02 11:28:47 2014 +1000
summary:     odbc: better handling of string parameters
C:\go\path\src\code.google.com\p\odbc>go vet ./...
c:\go\path\src\code.google.com\p\odbc\column.go:103: unreachable code
c:\go\path\src\code.google.com\p\odbc\mssql_test.go:227: database/sql.NullString
composite literal uses unkeyed fields
c:\go\path\src\code.google.com\p\odbc\mssql_test.go:235: database/sql.NullString
composite literal uses unkeyed fields
c:\go\path\src\code.google.com\p\odbc\mssql_test.go:308: arg is.weight for printf verb
%d of wrong type: float64
exit status 1
Maybe I am wrong thinking that "go vet" should hold my hand here. If not "go vet", then
I want some rules I can base my decisions on. I was hopping using "unsafe" would be less
like black magic at this moment. And, I expect, things will change again in the near
future.
Alex

@ianlancetaylor
Copy link
Contributor Author

Comment 14:

I agree with both of you: the spec is probably not the right place to write down these
rules, but the current rules must be written down somewhere.  I originally opened the
issue because the current state is confusing, and that remains the case.  I think we
should either reopen this issue as a non-spec issue, or open a new one.
This is related to issue #8310.

@griesemer
Copy link
Contributor

Comment 15:

I opened a new issue #8994.

@alexbrainman
Copy link
Member

Comment 16:

Sounds like a plan. Thank you, Ian.
Alex

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
Per suggestion from rsc as a result of the dicussion of
(abandoned) CL 153110044.

Fixes golang#7192.

LGTM=r, rsc, iant
R=r, rsc, iant, ken
CC=golang-codereviews
https://golang.org/cl/163050043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 26, 2018
Per suggestion from rsc as a result of the dicussion of
(abandoned) CL 153110044.

Fixes golang#7192.

LGTM=r, rsc, iant
R=r, rsc, iant, ken
CC=golang-codereviews
https://golang.org/cl/163050043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
Per suggestion from rsc as a result of the dicussion of
(abandoned) CL 153110044.

Fixes golang#7192.

LGTM=r, rsc, iant
R=r, rsc, iant, ken
CC=golang-codereviews
https://golang.org/cl/163050043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 20, 2018
Per suggestion from rsc as a result of the dicussion of
(abandoned) CL 153110044.

Fixes golang#7192.

LGTM=r, rsc, iant
R=r, rsc, iant, ken
CC=golang-codereviews
https://golang.org/cl/163050043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 30, 2018
Per suggestion from rsc as a result of the dicussion of
(abandoned) CL 153110044.

Fixes golang#7192.

LGTM=r, rsc, iant
R=r, rsc, iant, ken
CC=golang-codereviews
https://golang.org/cl/163050043
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants