Unsafe code, not a compiler bug.

It would be a minor bit of a work to fix this, and a larger bit of work to fix this in a way that did not leave useless local-zeroing in the code for correctly-written unsafe code.

@dr2chase Can you clarify what's incorrect about @kelseyfrancis's code? It looks valid to me.

@mdempsky:

    var x uint64
    *(*uint16)(unsafe.Pointer(&x)) = 0xffee

That's treating a uint64 as a uint16.

Right, I see that. What's invalid about that? I don't recall package unsafe ever saying type punning like that isn't allowed in Go.

Package unsafe has also never promised it is valid. As @randall77 has said elsewhere, we're allowed to change the unsafe behavior between releases because because it makes no promises.

https://golang.org/doc/go1compat even says:

Use of package unsafe. Packages that import unsafe may depend on internal properties of the Go implementation. We reserve the right to make changes to the implementation that may break such programs.

Fair enough. Incidentally, explicit initialization of x produces the same result:

    var x uint64 = 0x1020304050607080
    *(*uint16)(unsafe.Pointer(&x)) = 0xffee
    fmt.Printf("%x\n", x)  // prints c42000ffee rather than 102030405060ffee

https://golang.org/pkg/unsafe/#Pointer says:

(1) Conversion of a *T1 to Pointer to *T2.

Provided that T2 is no larger than T1 and that the two share an equivalent memory layout, this conversion allows reinterpreting data of one type as data of another type.

My interpretation of "equivalent memory layout" is referring to having the same pointer slots. Since uint16 is no larger than uint64 and neither contain pointers, it seems explicitly documented to be valid.

If not, we should clarify the documentation to define "memory layout". That term isn't used in the Go language spec or memory model.

Reopening because at the very least I think there's a documentation issue here.

We should fix this. The failure mode is bad - I believe we're exposing uninitialized stack memory this way. That's a (very minor, but still) security bug.

Workaround is to assign the local to a global sink before the unsafe use; this will force the zeroing to occur.

Just treat unsafe stores as not real stores, and thus no redundant store elimination?
Would only hurt code that ought not use unsafe anyway, since it has to be locals anyhow.

Keith, how can use of unsafe not be a security violation in general?
What prevents taking address of auto, casting to *byte[100000], and happily iterating over it?

+1 on @dr2chase's point on security.

Anyway, I send CL https://go-review.googlesource.com/c/27320/.

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

If we're revising the documentation for unsafe, can we make it a lot scarier and with even fewer things that look like promises?

@dr2chase I thought the only promise was "We promise to occasionally break your use of unsafe."

occasionally
and it was already broken, you were just deluded into thinking it wasn't by some spuriously passing test.

Keith, how can use of unsafe not be a security violation in general?

Package unsafe documents several patterns that users are allowed to assume will remain safe, one of which includes (at least by my best interpretation) converting *uint64 to *uint16. If there were no safe uses of package unsafe, then there wouldn't be any point in providing it.

What prevents taking address of auto, casting to *byte[100000], and happily iterating over it?

That's not one of the patterns that are documented to be safe. Conversion of *T1 to *T2 is only safe if Sizeof(T2) <= Sizeof(T1), which won't be true in the scenario you're asking about (since we don't allocate 100,000+ byte objects on the stack).

and it was already broken,

Do you mean that the pre-SSA compiler didn't support conversions from *uint64 to *uint16 either? Or you think the user code was broken for relying on patterns that you don't agree should be valid?

I think that we should not be bound by any particular artifacts of the previous compiler's treatment of unsafe code, because the full set of those artifacts is not specified. I also think it was a mistake to "document" this behavior for end-user use, because people will write unsafe code that happens to pass tests with a particular Go release and believe that therefore they got it right. We don't have a test suite for what unsafe behavior we support except for the runtime itself, and compiler writers aren't good at imaging the crazy things that people might do with unsafe code. And lacking such a test suite, for unsafe, I think it's fine for compiler-writers to push back hard on "bugs" involving unsafe code.

Unsafe code is generally a security violation, because no matter what we say in documentation, we don't check it mechanically. It's pretty much the whole point of actual security violations that they do an end-run on documented restrictions. Or as I asked, "what prevents?"

Reopening to track backporting to Go 1.7.1. IMO this is a regression from 1.6, and if we're going to fix for 1.8, we should backport to 1.7.1 too. /cc @broady

@dr2chase I'll followup on golang-dev. I think this discussion is worth having, just it shouldn't be here. :)

Closing because this is fixed on master. Per new release policy, a separate issue will track cherry-picking for Go 1.7.1.

@mdempsky, just curious what is the new issue number or has it not been created yet?

@randall77, we're getting stuck cherry-picking this to the Go 1.7 branch. It seems to depend on some earlier stuff.

Rather than @broady and I botch it, can you prepare a version of the fix for the 1.7 release branch?

https://go-review.googlesource.com/28670 (manual cherrypick of 27320 into release-branch.go1.7) out for review.