Skip to content

batchverifier: preserve the memory until the end#4672

Merged
algorandskiy merged 2 commits into
algorand:masterfrom
algonautshant:shant/batch-verifier-mem-issue
Oct 19, 2022
Merged

batchverifier: preserve the memory until the end#4672
algorandskiy merged 2 commits into
algorand:masterfrom
algonautshant:shant/batch-verifier-mem-issue

Conversation

@algonautshant
Copy link
Copy Markdown
Contributor

In batchVerificationImpl, the signatures, messages and publicKeys are not copied. The memory from the Go slice is used by the C code, by creating C pointers to these memory location. However, Go is not aware of this usage, and can recover the memory used by these, since no Go variable is referencing them. This may happen when the C code is evaluating them, resulting is accessing corrupted memory.
This fix tells Go to keep alive these references.

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 19, 2022

Codecov Report

Merging #4672 (b4ffa76) into master (3f1f3e4) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4672      +/-   ##
==========================================
+ Coverage   54.39%   54.43%   +0.04%     
==========================================
  Files         403      403              
  Lines       51921    51924       +3     
==========================================
+ Hits        28240    28263      +23     
+ Misses      21300    21285      -15     
+ Partials     2381     2376       -5     
Impacted Files Coverage Δ
crypto/batchverifier.go 100.00% <100.00%> (ø)
network/wsPeer.go 66.50% <0.00%> (-1.95%) ⬇️
network/wsNetwork.go 65.71% <0.00%> (+0.36%) ⬆️
ledger/tracker.go 78.72% <0.00%> (+0.85%) ⬆️
catchup/peerSelector.go 100.00% <0.00%> (+1.04%) ⬆️
catchup/service.go 69.38% <0.00%> (+1.23%) ⬆️
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️
ledger/blockqueue.go 88.50% <0.00%> (+2.87%) ⬆️
ledger/roundlru.go 96.22% <0.00%> (+5.66%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cce
Copy link
Copy Markdown
Contributor

cce commented Oct 19, 2022

I believe we are doing a correct conversion from uintptr to Pointer when we process the input slices ... doesn't that mean Go will recognize unsafe.Pointer is a reference for GC purposes?

https://utcc.utoronto.ca/~cks/space/blog/programming/GoUintptrVsUnsafePointer

Although unsafe.Pointers are generic pointers, the Go garbage collector knows that they point to Go objects; in other words, they are real Go pointers. Through internal magic, the garbage collector can and will use them to keep live objects from being reclaimed and to discover further live objects (if the unsafe.Pointer points to an object that has pointers of its own). Due to this, a lot of the restrictions on what you can legally do with unsafe.Pointers boil down to 'at all times, they must point to real Go objects'. If you create an unsafe.Pointer that doesn't, even for a brief period of time, the Go garbage collector may choose that moment to look at it and then crash because it found an invalid Go pointer.

https://groups.google.com/g/golang-nuts/c/yNis7bQG_rY/m/yaJFoSx1hgIJ

If the unsafe.Pointer points to a Go object, the garbage collector will know about the object and the memory belonging to the object will not be freed.

From https://pkg.go.dev/unsafe#Pointer

The remaining patterns enumerate the only valid conversions from uintptr to Pointer.

(3) Conversion of a Pointer to a uintptr and back, with arithmetic.

If p points into an allocated object, it can be advanced through the object by conversion to uintptr, addition of an offset, and conversion back to Pointer.

p = unsafe.Pointer(uintptr(p) + offset)

The most common use of this pattern is to access fields in a struct or elements of an array:

// equivalent to f := unsafe.Pointer(&s.f)
f := unsafe.Pointer(uintptr(unsafe.Pointer(&s)) + unsafe.Offsetof(s.f))

// equivalent to e := unsafe.Pointer(&x[i])
e := unsafe.Pointer(uintptr(unsafe.Pointer(&x[0])) + i*unsafe.Sizeof(x[0]))

It is valid both to add and to subtract offsets from a pointer in this way. It is also valid to use &^ to round pointers, usually for alignment. In all cases, the result must continue to point into the original allocated object.

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 this pull request may close these issues.

3 participants