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

Invalid use of reflect.SliceHeader and reflect.StringHeader could lead to memory corruption #185

Closed
lthibault opened this issue Oct 29, 2021 · 4 comments · Fixed by #186
Closed
Labels

Comments

@lthibault
Copy link
Collaborator

In integration_test.go, the following is flagged by go vet as a possible misuse of reflect.SliceHeader and reflect.StringHeader.

A cursory google search supports the notion that this is a bug.

func unsafeBytesToString(b []byte) string {
	slice := *(*reflect.SliceHeader)(unsafe.Pointer(&b))
	hdr := reflect.StringHeader{Data: slice.Data, Len: slice.Len}
	return *(*string)(unsafe.Pointer(&hdr))
}

Two questions:

  1. Couldn't this be simplified to *(*string)(unsafe.Pointer(&b))?
  2. I appreciate that this is running in benchmarking code, but perhaps we should just opt for a good ol' fashioned string(b) here?

The impact is sure to be minor since this is just running in tests, but (a) I prefer to do things the right way and (b) I don't like it when my linter yells at me.

Thoughts?

@lthibault lthibault added the bug label Oct 29, 2021
@lthibault
Copy link
Collaborator Author

cc @zenhack

@zenhack
Copy link
Contributor

zenhack commented Oct 29, 2021

  1. Yes, I think this simplification is correct.
  2. It seems likely that that would dominate the benchmark, so I'm hesitant to just do that. I wish the intentions of the benchmark were more explicitly spelled out; it would make it easier to try to find a safe way to write something that does the right thing.

@lthibault
Copy link
Collaborator Author

Ok, I'll just apply the *(*string)(unsafe.Pointer(&b)) correction.

@lthibault
Copy link
Collaborator Author

Closing optimistically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants