search

valyala / gozstd

go wrapper for zstd
MIT License
434 stars 63 forks source link

Avoid cgo bug by use C.calloc to allocate memory in C instead of C.ma… #61

Closed waldoweng closed 6 months ago

waldoweng commented 7 months ago

…lloc,

See https://pkg.go.dev/cmd/cgo:

Note: the current implementation has a bug. While Go code is permitted to write nil or a C pointer (but not a Go pointer) to C memory, the current implementation may sometimes cause a runtime error if the contents of the C memory appear to be a Go pointer. Therefore, avoid passing uninitialized C memory to Go code if the Go code is going to store pointer values in it. Zero out the memory in C before passing it to Go.

I also find similar approach use by Dgraph according to this blog post: https://dgraph.io/blog/post/manual-memory-management-golang-jemalloc/

So, instead of using malloc, we use its slightly more expensive sibling, calloc. calloc works the same way as malloc, except it zeroes out the memory before returning it to the caller.

f41gh7 commented 6 months ago

Hello, do you have any examples with panic related to this lib?

As far as I understand current issue with CGO. It's only possible to get a panic, if program makes allocation of uninitialized objects via malloc and shadows it with GO allocations. Usual example for it is slice resizing via append.

f41gh7 commented 6 months ago

Related issue https://github.com/golang/go/issues/19928

valyala commented 6 months ago

@waldoweng , thanks for the fix!