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

proposal: crypto/sha256: satisfy io.StringWriter #34904

Closed
hummerd opened this issue Oct 14, 2019 · 4 comments
Closed

proposal: crypto/sha256: satisfy io.StringWriter #34904

hummerd opened this issue Oct 14, 2019 · 4 comments
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone
Proposal

Comments

@hummerd
Copy link

hummerd commented Oct 14, 2019 •
edited

Hi, everyone!
Let's implement io.StringWriter for sha256 digest.

// Yes this looks dangerous
// But there is no way for d.Write(p) to change p (now or ever) 
// since this will break backward compatability, also we can guarantee
// imutability of s with test
func (d *digest) WriteString(s string) (nn int, err error) {
	hString := (*reflect.StringHeader)(unsafe.Pointer(&s))
	hSlice := &reflect.SliceHeader{
		Data: hString.Data,
		Len:  hString.Len,
		Cap:  hString.Len,
	}
	p := *(*[]byte)(unsafe.Pointer(hSlice))
	return d.Write(p)
}

This will let us execute this benchmarks 17% faster and with 0 allocs/op (tested with go 13.1)

var testString string = "some very very long string that will cause allocation, when converted to []byte"

// regular string hashing
func BenchmarkHash(b *testing.B) {
	h := sha256.New()

	for i := 0; i < b.N; i++ {
		_, _ = h.Write([]byte(testString))
	}
}

// string hashing with io.StringWriter
func BenchmarkHashString(b *testing.B) {
	h := sha256.New()
	sw := h.(io.StringWriter)

	for i := 0; i < b.N; i++ {
		_, _ = sw.WriteString(testString)
	}
}
@gopherbot gopherbot added this to the Proposal milestone Oct 14, 2019
@agnivade agnivade added the Proposal-Crypto Proposal related to crypto packages or other security issues label Oct 15, 2019
@agnivade agnivade changed the title proposal: Some tricky optimization in crypto sha256 proposal: crypto/sha256: satisfy io.StringWriter Oct 15, 2019
@josharian
Copy link
Contributor

Also proposed at #14757

@ainar-g
Copy link
Contributor

ainar-g commented Oct 17, 2019

This is wildly unsafe: 

p := *(*[]byte)(unsafe.Pointer(&s))

See unsafe rule №1:

(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, (…)

In fact, this very same comment shows exactly how this conversion must be done: 

var s string
hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) // case 1
hdr.Data = uintptr(unsafe.Pointer(p))              // case 6 (this case)
hdr.Len = n

@hummerd
Copy link
Author

hummerd commented Oct 17, 2019 •
edited

This is wildly unsafe:

p := *(*[]byte)(unsafe.Pointer(&s))

See unsafe rule №1:

(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, (…)

In fact, this very same comment shows exactly how this conversion must be done:

var s string
hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) // case 1
hdr.Data = uintptr(unsafe.Pointer(p))              // case 6 (this case)
hdr.Len = n

Changed WriteString implementation to more correct one with proper capacity initialization

@FiloSottile
Copy link
Contributor

Closing as duplicate of #14757. It would be useful to mention real world use cases for it in that issues.

@golang golang locked and limited conversation to collaborators Dec 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
6 participants
@josharian @FiloSottile @agnivade @hummerd @ainar-g @gopherbot