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.

Sign up for GitHub

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

Jump to bottom

cmd/cgo: recognize that unsafe.StringData only points to a string #59954

Closed
ianlancetaylor opened this issue May 3, 2023 · 4 comments
Closed

cmd/cgo: recognize that unsafe.StringData only points to a string #59954

ianlancetaylor opened this issue May 3, 2023 · 4 comments
Assignees
@ianlancetaylor
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.21

Comments

@ianlancetaylor
Copy link
Contributor

After https://go.dev/cl/483815 this program fails when run with GOEXPERIMENT=boringcrypto:

package main

import (
	"crypto/sha1"
	"fmt"
	"io"
	"unsafe"
)

func hash(a, b string) []byte {
	h := sha1.New()
	h.(io.StringWriter).WriteString(a+b)
	return h.Sum(nil)
}

type S struct {
	buf [5]byte
	z   int
	p   *int
}

var x = S{p: new(int)}

func main() {
	x.buf[0] = 'a'
	s := unsafe.String(&x.buf[0], 1)
	fmt.Printf("%x\n", hash(s, ""))
}

Running this program normally works fine. With GOEXPERIMENT=boringcrypto it fails with

panic: runtime error: cgo argument has Go pointer to Go pointer

goroutine 1 [running]:
crypto/internal/boring.(*sha1Hash).WriteString.func1(0xc00009e000, {0x6fbcf0, 0x1})
	/home/iant/go/src/crypto/internal/boring/sha.go:149 +0x30
crypto/internal/boring.(*sha1Hash).WriteString(0xc0000061a0?, {0x6fbcf0?, 0x1})
	/home/iant/go/src/crypto/internal/boring/sha.go:149 +0x25
main.hash({0x6fbcf0, 0x1}, {0x0, 0x0})
	/home/iant/foo.go:12 +0x85
main.main()
	/home/iant/foo.go:27 +0x3d
exit status 2

The problem is that the boringcrypto code looks like this:

func (h *sha1Hash) WriteString(s string) (int, error) {
	if len(s) > 0 && C._goboringcrypto_SHA1_Update(h.noescapeCtx(), unsafe.Pointer(unsafe.StringData(s)), C.size_t(len(s))) == 0 {
		panic("boringcrypto: SHA1_Update failed")
	}
	return len(s), nil
}

The cgo command does not recognize that passing unsafe.Pointer(unsafe.StringData(s)) should only check the contents of the string. Since it doesn't recognize that, it winds up checking the entire contents of the struct that contains the string, leading to report an incorrect error.

While this test case is rather artificial, a more realistic test case can be constructed using arenas, in which the arena itself plays the part of the struct that may have associated pointers.
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 3, 2023
@gopherbot
Copy link

Change https://go.dev/cl/492355 mentions this issue: Revert "crypto/sha512: add WriteString and WriteByte method"

@gopherbot
Copy link

Change https://go.dev/cl/492375 mentions this issue: Revert "crypto/sha1: add WriteString and WriteByte method"

@gopherbot
Copy link

Change https://go.dev/cl/492356 mentions this issue: Revert "crypto/sha256: add WriteString and WriteByte method"

gopherbot pushed a commit that referenced this issue May 3, 2023
@ianlancetaylor @gopherbot
Revert "crypto/sha512: add WriteString and WriteByte method"
5b09a28 
This reverts CL 483816

Reason for revert: can cause cgo errors when using boringcrypto.  See #59954.

For #38776
For #59954

Change-Id: I23a2a1f0aed2a08b73855b5038ccb24a4d0a02c0
Reviewed-on: https://go-review.googlesource.com/c/go/+/492355
Run-TryBot: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue May 3, 2023
@ianlancetaylor @gopherbot
Revert "crypto/sha256: add WriteString and WriteByte method"
82629da 
This reverts CL 481478

Reason for revert: can cause cgo errors when using boringcrypto.
See #59954.

For #38776
For #59954

Change-Id: Ic520f9fede152d22ab69996ad84c44f3e0d783bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/492356
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
gopherbot pushed a commit that referenced this issue May 3, 2023
@ianlancetaylor @gopherbot
Revert "crypto/sha1: add WriteString and WriteByte method"
129b597 
This reverts CL 483815

Reason for revert: can cause cgo errors when using boringcrypto.
See #59954.

For #38776
For #59954

Change-Id: I1f7e0fb06b627971811623927e3d98c0fdbc730b
Reviewed-on: https://go-review.googlesource.com/c/go/+/492375
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Bypass: Ian Lance Taylor <iant@google.com>
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 3, 2023
@gopherbot
Copy link

Change https://go.dev/cl/493556 mentions this issue: cmd/cgo: recognize unsafe.{StringData,SliceData}

@mknyszek mknyszek modified the milestones: Backlog, Go1.21 May 10, 2023
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ianlancetaylor ianlancetaylor

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
5 participants
@cagedmantis @mknyszek @dmitshur @ianlancetaylor @gopherbot