[Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation
Laszlo Ersek
lersek at redhat.com
Mon Sep 20 15:11:38 UTC 2021
On 09/20/21 12:37, Daniel P. Berrangé wrote:
> On Mon, Sep 20, 2021 at 07:23:35AM +0200, Laszlo Ersek wrote:
>> The current "arg_string_list" and "free_string_list" implementations go
>> back to commit b6f01f32606d ("Add Go (language) bindings.", 2013-07-03).
>> There are two problems with them:
>>
>> - "free_string_list" doesn't actually free anything,
>
> Doh.
In fact I do not think that was an oversight (i.e. that C.free() was
commented out). I didn't try, but I suspect that calling C.free()
triggered the same issue when Rich originally worked on the go bindings,
so C.free() was postponed. And then forgotten. (This is just my
speculation, again.)
>
>>
>> - at the *first* such g.Internal_test() call site that passes an
>> Ostringlist member inside the Optargs argument, namely:
>>
>>> g.Internal_test ("abc",
>>> string_addr ("def"),
>>> []string{},
>>> false,
>>> 0,
>>> 0,
>>> "123",
>>> "456",
>>> []byte{'a', 'b', 'c', '\000', 'a', 'b', 'c'},
>>> &guestfs.OptargsInternal_test{Ostringlist_is_set: true,
>>> Ostringlist: []string{}
>>> }
>>> )
>>
>> the "golang/run-bindtests" case crashes:
>>
>>> panic: runtime error: cgo argument has Go pointer to Go pointer
>>>
>>> goroutine 1 [running]:
>>> libguestfs.org/guestfs.(*Guestfs).Internal_test.func7(0xc000018180,
>>> 0xadfb60, 0xadfb80, 0xc000010048, 0x0, 0x0, 0x0, 0xae3e10, 0xae3e30,
>>> 0xade3a0, ...)
>>> golang/src/libguestfs.org/guestfs/guestfs.go:6729 +0xa9
>>> libguestfs.org/guestfs.(*Guestfs).Internal_test(0xc000018180, 0x4ee3a5,
>>> 0x3, 0xc000061be8, 0xc000061af8, 0x0, 0x0, 0xc000061a00, 0x0, 0x0, ...)
>>> golang/src/libguestfs.org/guestfs/guestfs.go:6729 +0x3c9
>>> main.main()
>>> golang/bindtests/bindtests.go:77 +0x151e
>>> exit status 2
>>> FAIL run-bindtests (exit status: 1)
>
>
> What distro / go version do you see this on, as I can't reproduce
> this pointer problem with a standalone demo app ?
Stock Fedora 34:
golang-bin-1.16.6-2.fc34.x86_64
It's reproduced reliably by
$ make -C golang V=1 check
in libguestfs.
Interestingly, this kind of "stringlist" is used in two distinct
scenarios in "golang/bindtests/bindtests.go". The first one is when such
a stringlist is passed to the generated Internal_test() function
[golang/src/libguestfs.org/guestfs/guestfs.go] through the "strlist"
parameter. There are no problems then. But when a similar object is
embedded in the guestfs.OptargsInternal_test argument, that's when
things break.
>> It turns out that a C-language array containing C-language pointers can
>> only be allocated in C:
>>
>> https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
>>
>> Rewrite the "arg_string_list" and "free_string_list" functions almost
>> entirely in C.
>>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
>> ---
>> generator/golang.ml | 46 ++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 37 insertions(+), 9 deletions(-)
>>
>> diff --git a/generator/golang.ml b/generator/golang.ml
>> index d328abe4ed08..5db478e8a494 100644
>> --- a/generator/golang.ml
>> +++ b/generator/golang.ml
>> @@ -52,6 +52,39 @@ _go_guestfs_create_flags (unsigned flags)
>> {
>> return guestfs_create_flags (flags);
>> }
>> +
>> +//
>> +// A C-language array of C-language pointers needs to be allocated in C.
>> +// https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
>> +//
>> +typedef char *pChar;
>> +
>> +pChar *allocStringArray (size_t nmemb)
>> +{
>> + pChar *array;
>> +
>> + array = malloc (sizeof (pChar) * (nmemb + 1));
>> + array[nmemb] = NULL;
>> + return array;
>> +}
>> +
>> +void storeString (pChar *array, size_t idx, pChar string)
>> +{
>> + array[idx] = string;
>> +}
>> +
>> +void freeStringArray (pChar *array)
>> +{
>> + pChar *position;
>> + pChar string;
>> +
>> + position = array;
>> + while ((string = *position) != NULL) {
>> + free (string);
>> + position++;
>> + }
>> + free (array);
>> +}
>> */
>> import \"C\"
>>
>> @@ -148,12 +181,11 @@ func (g *Guestfs) Close () *GuestfsError {
>> * C strings and golang []string.
>> */
>> func arg_string_list (xs []string) **C.char {
>> - r := make ([]*C.char, 1 + len (xs))
>> + r := C.allocStringArray (C.size_t (len (xs)))
>> for i, x := range xs {
>> - r[i] = C.CString (x)
>> + C.storeString (r, C.size_t (i), C.CString (x));
>> }
>> - r[len (xs)] = nil
>> - return &r[0]
>> + return (**C.char) (r)
>> }
>
> This could be done purely in Go without helper functions
>
> func array_elem(arr **C.char, idx int) **C.char {
> return (**C.char)(unsafe.Pointer(uintptr(unsafe.Pointer(arr)) +
> (unsafe.Sizeof(arr) * uintptr(idx))))
> }
>
> func array_size(arr **C.char, len int) C.size_t {
> return C.size_t(unsafe.Sizeof(*arr) * (1 + uintptr(len)))
> }
>
> func arg_string_list2(xs []string) **C.char {
> var r **C.char
> r = (**C.char)(C.malloc(array_size(len(xs))))
> for i, x := range xs {
> str := array_elem(r, i)
> *str = C.CString(x)
> }
> str := array_elem(r, len(xs))
> *str = nil
> return r
> }
I'm a total noob in Go and I find the syntax very difficult to read. In
addition to that, I don't have the slightest idea about the Go runtime.
(What I do know however is that the #cgo parser is not a full-blown C
language parser; for example it does not accept "sizeof variable"
without parens, plus see <https://github.com/golang/go/issues/14210> for
another example.) The end result is that I wanted to get out of Go-land
as quickly and as *simply* as possible (meaning the syntax of calling C
code), and then to do the actual business in C. This is the Go binding
of a C library, so I feel this approach is justified.
I concede that the commit message / comments that state that the array
"must" be allocated like this may be a stretch. Note that it's not
without precedent, see this other comment in "generator/golang.ml":
// cgo can't deal with variable argument functions.
Now another comment in <https://github.com/golang/go/issues/14210>, from
Ian Lance Taylor, is:
That is the way the current implementation works, a consequence of
the way that cgo rewrites calls to C functions. We can try to do
better for 1.7, but we aren't going to change this for 1.6.
So the language runtime is even a moving target, and it's not stable
what can be done and what cannot be done in it. I think the code is
justified; perhaps the comments could be relaxed: "it's best to allocate
arrays like this".
Indeed I don't know *why* Go breaks like that, so I cannot capture that
reason in the commit message. On the other hand, I don't know any other
reasons either why Go does its things the way it does -- and I still
wanted to fix this bug.
I'm happy to pass on this issue to someone else, or (if you can propose
an exact language for the commit message and the code comment) spin a v2
of this patch. Personally I wouldn't like to modify the *code* in the patch.
>
>
>>
>> func count_string_list (argv **C.char) int {
>> @@ -167,11 +199,7 @@ func count_string_list (argv **C.char) int {
>> }
>>
>> func free_string_list (argv **C.char) {
>> - for *argv != nil {
>> - //C.free (*argv)
>> - argv = (**C.char) (unsafe.Pointer (uintptr (unsafe.Pointer (argv)) +
>> - unsafe.Sizeof (*argv)))
>> - }
>> + C.freeStringArray ((*C.pChar) (argv))
>> }
>
> This can be done entirely in Go
>
> func free_string_list (argv **C.char) {
> for i := 0; ; i++ {
> str := array_elem(argv, i)
> if *str == nil {
> break
> }
> C.free(unsafe.Pointer(*str))
> }
> C.free(unsafe.Pointer(argv))
> }
I guess it "can", but *should* it? In libguestfs? I think (for any
language binding at all, not just Go's), the bindings should be as thin
as possible in the other languages, and most of the business should be in C.
Thanks,
Laszlo
>
>>
>> func return_string_list (argv **C.char) []string {
>> --
>> 2.19.1.3.g30247aa5d201
>>
>> _______________________________________________
>> Libguestfs mailing list
>> Libguestfs at redhat.com
>> https://listman.redhat.com/mailman/listinfo/libguestfs
>>
>
> Regards,
> Daniel
>
More information about the Libguestfs
mailing list