Skip to content

Commit

Permalink
Fix bytesToStr (#358)
Browse files Browse the repository at this point in the history
The current implementation of bytesToStr uses an unsafe
reflect.StringHeader value. Change the implementation of this function
to a safe and simpler version.

To explain what could go wrong here is some example code:

  var d []byte
  d = someFunctionThatReturnsBytes()

  s := bytesToStr(d)

  doSomethingWith(s)

When this code gets compiled bytesToStr would get inlined and the
code would be like the following. I have included in comments at which
point things could go wrong:

  var d []byte
  d = someFunctionThatReturnsBytes()

  h := (*reflect.SliceHeader)(unsafe.Pointer(&d))
  shdr := reflect.StringHeader{Data: h.Data, Len: h.Len}

  // At this point in time d and d.Data have nothing referencing them anymore
  // shdr.Data is an uintptr so it will be ignored by the GC.
  // This means d and d.Data can be garbage collected here.
  // Internally strings don't use a uintptr for the data, but since this is
  // just a reflect.StringHeader and not a real string yet that doesn't apply
  // here.
  // This is why https://pkg.go.dev/unsafe#Pointer says:
  //   In general, reflect.SliceHeader and reflect.StringHeader should be
  //   used only as *reflect.SliceHeader and *reflect.StringHeader pointing
  //   at actual slices or strings, never as plain structs.

  s := *(*string)(unsafe.Pointer(&shdr))

  // Only at this point s.Data points to d.Data again and the backing storage
  // of d won't be garbage collected anymore.

  doSomethingWith(s)

The chance of this going wrong is probably so small that nobody ever
noticed it happening, but it is there.
  • Loading branch information
erikdubbelboer committed Apr 4, 2022
1 parent 11c9d7f commit a209843
Showing 1 changed file with 1 addition and 4 deletions.
5 changes: 1 addition & 4 deletions jlexer/bytestostr.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
package jlexer

import (
"reflect"
"unsafe"
)

Expand All @@ -18,7 +17,5 @@ import (
// chunk may be either blocked from being freed by GC because of a single string or the buffer.Data
// may be garbage-collected even when the string exists.
func bytesToStr(data []byte) string {
h := (*reflect.SliceHeader)(unsafe.Pointer(&data))
shdr := reflect.StringHeader{Data: h.Data, Len: h.Len}
return *(*string)(unsafe.Pointer(&shdr))
return *(*string)(unsafe.Pointer(&data))
}

0 comments on commit a209843

Please sign in to comment.