Fixes #43
Fixes #43
Conversation
| func (c *Context) Verify(sig, signedText, plain *Data) (string, []Signature, error) { | ||
| var pinner runtime.Pinner | ||
| pinner.Pin(unsafe.Pointer(c)) | ||
| pinner.Pin(unsafe.Pointer(sig)) |
There was a problem hiding this comment.
Just curious — is this a cleanup, or is this intended to fix a bug? (We are running into some unexplained file descriptor closures, so I’d be interested to learn about any misconceptions I might have about how the Go/CGo interaction works.)
There was a problem hiding this comment.
Cleanup. The docs say pinner prevent the go memory from being "moved", so if go does moves right now, then pinner could prevent some bugs compared to just using runtime.KeepAlive. Unexplained file descriptor closures could be explained missing keep-alives. Would need more diagnosis.
I think ideally pinner would be embedded into the struct to save the duplication and reduce likelihood of missing calls, but I don't yet understand how it interacts with finalizers. In retrospect I don't think I would have used finalizers in this wrapper. More trouble than they're worth.
There was a problem hiding this comment.
I still don't know what causes the failure on mac. I need to get a mac to better diagnose. I'm wondering if the error handling is inadequate in some way.
There was a problem hiding this comment.
Thanks!
The docs say pinner prevent the go memory from being "moved", so if go does moves right now, then pinner could prevent some bugs compared to just using runtime.KeepAlive.
I read that to mean that it prevents moving the c/sig structs; that shouldn’t directly affect the C-visible pointer values of c.ctx/sig.dh, hopefully the Go runtime is not moving C structures it didn’t create. (A second side-effect is that it prevents freeing, so it should be a superset of runtime.KeepAlive.)
For the Go-native objects passed to C, the current code seems to be already be using cgo.Handle, so moving shouldn’t make any difference…
[… well… as documented, the code is correctly passing a *cgo.Handle to C. Nothing says that the target of the pointer needs to, or doesn’t need to, be pinned using a runtime.Pinner. Interesting, something I should study more.]
Unexplained file descriptor closures could be explained missing keep-alives.
Yes. At this point we don’t have a reliable reproducer, and it’s in a long-running process with many dependencies; it’s very possible that there is no relationship to gpgme. (cri-o/cri-o#8906 , for the record.)
There was a problem hiding this comment.
I still don't know what causes the failure on mac. I need to get a mac to better diagnose. I'm wondering if the error handling is inadequate in some way.
I have tried running the tests on macOS locally (with GPG 2.4.8 and modifying ctxWithCallback to ctx.SetPinEntryMode(PinEntryLoopback)) and 1000 iterations all succeeded for me.
No description provided.