clone: do not free clone options' payload #242

2015-08-12T05:56:55-05:00

pks-t commented

2015-08-12 05:56:55 -05:00

(Migrated from github.com)

The void pointer that is being passed when populating the clone
options' payload is currently being freed as soon as the
populating function returns. As the memory it points to is only
ever being accessed after the function returns this will cause
invalid memory access.

Fix this by removing the call to free. As we cannot know
what the caller is passing as content of the void pointer we
cannot take responsibility for freeing it. This has to be the
responsibility of the caller, instead.

The `void` pointer that is being passed when populating the clone options' payload is currently being freed as soon as the populating function returns. As the memory it points to is only ever being accessed after the function returns this will cause invalid memory access. Fix this by removing the call to `free`. As we cannot know what the caller is passing as content of the `void` pointer we cannot take responsibility for `free`ing it. This has to be the responsibility of the caller, instead.

carlosmn commented

2015-08-12 19:02:17 -05:00

(Migrated from github.com)

Can you actually use this functionality from Go?

pks-t commented

2015-08-13 05:15:17 -05:00

(Migrated from github.com)

Well, it is possible to do it. The caller has to create a Go function that is exported via Cgo, though, which is very uncomfortable. Guess I'll code up a new version that exposes this functionality in a saner way.

carlosmn commented

2015-08-13 05:47:10 -05:00

(Migrated from github.com)

I was thinking more of even using the type. Once I tried to use a subpackage in git2go (for the global settings, IIRC) and the compiler wouldn't agree on sharing the C types across them.

pks-t commented

2015-08-13 07:13:34 -05:00

(Migrated from github.com)

Didn't see your comment until now, so regard this as a RFC.
I've changed the callback field to accept a Go function instead, that is being passed the parameters and may handle creation of the default remote. As usual, it is quite complicated when wrapping functions that have C callbacks, but the provided version works as expected (at least for Go > v1.2). I don't know yet why it won't compile for the older versions, though, but I'll go hunt that down.

Didn't see your comment until now, so regard this as a RFC. I've changed the callback field to accept a Go function instead, that is being passed the parameters and may handle creation of the default remote. As usual, it is quite complicated when wrapping functions that have C callbacks, but the provided version works as expected (at least for Go > v1.2). I don't know yet why it won't compile for the older versions, though, but I'll go hunt that down.

carlosmn commented

2015-08-14 14:48:54 -05:00

(Migrated from github.com)

Moving to a Go callback would be great. Note that this means the payload should be removed. That's just there in C in order to emulate closures, but Go provides them in the language, so we don't need that.

pks-t commented

2015-08-18 07:06:08 -05:00

(Migrated from github.com)

Go1.1 does not allow assigning C function pointers to variables in Go code
Go1.2 does not allow C-pointers to structs in exported functions

I've worked around those issues by using a C function that populates the callback function inside the struct and changing the function's signature to accept unsafe.Pointers, only.
I've also removed the ability to explicitly set the payload. Instead, a Go closure should be used.

- Go1.1 does not allow assigning C function pointers to variables in Go code - Go1.2 does not allow C-pointers to structs in exported functions I've worked around those issues by using a C function that populates the callback function inside the struct and changing the function's signature to accept unsafe.Pointers, only. I've also removed the ability to explicitly set the payload. Instead, a Go closure should be used.

Sign in to join this conversation.

No reviewers