[Libguestfs] [PATCH v2] Go bindings: fix "C array of strings" -- char** -- allocation

[Laszlo Ersek]

On 09/21/21 23:01, Richard W.M. Jones wrote:
> On Tue, Sep 21, 2021 at 09:29:39PM +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,
>>
>> - 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)
>>
>> In Daniel P. Berrangé's words [1],
>>
>>> You're allowed to pass a Go pointer to C via CGo, but the memory that
>>> points to is not allowed to contained further Go pointers. So the struct
>>> fields must strictly use a C pointer.
>>
>> One pattern to solve the problem has been shown on stackoverflow [2].
>> Thus, rewrite the "arg_string_list" and "free_string_list" functions
>> almost entirely in C, following that example.
>>
>> While this approach is not the most idiomatic Go, as a solution exists
>> without C helper functions [3], it should still be acceptable, at least as
>> an incremental improvement, for letting "golang/run-bindtests" pass.
>>
>> [1] https://listman.redhat.com/archives/libguestfs/2021-September/msg00118.html
>> [2] https://stackoverflow.com/questions/35924545/golang-cgo-panic-runtime-error-cgo-argument-has-go-pointer-to-go-pointer
>> [3] https://listman.redhat.com/archives/libguestfs/2021-September/msg00106.html
>>
>> Cc: "Daniel P. Berrangé" <berrange at redhat.com>
>> Cc: "Richard W.M. Jones" <rjones at redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> 
> This time I enabled the golang bindings, reproduced the bug you
> reported, applied this patch, reran the tests and verified that it has
> been fixed, so:
> 
> Tested-by: "Richard W.M. Jones" <rjones at redhat.com>
> Acked-by: "Richard W.M. Jones" <rjones at redhat.com>

Commit e2f8db27d0af.

Thank you!
Laszlo

> 
> Rich.
> 
>>
>> Notes:
>>     v2:
>>     - update commit message [Daniel, Rich]
>>     - update code comment [Daniel, Rich]
>>     - no code changes
>>     - retested
>>
>>  generator/golang.ml | 47 ++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 38 insertions(+), 9 deletions(-)
>>
>> diff --git a/generator/golang.ml b/generator/golang.ml
>> index d328abe4ed08..0d6a92367f9b 100644
>> --- a/generator/golang.ml
>> +++ b/generator/golang.ml
>> @@ -52,6 +52,40 @@ _go_guestfs_create_flags (unsigned flags)
>>  {
>>      return guestfs_create_flags (flags);
>>  }
>> +
>> +// Passing a Go pointer to C via CGo is allowed, but the memory that points to
>> +// is not allowed to contain further Go pointers. The helper functions below
>> +// are one way to implement this, although the same can be achieved purely in
>> +// Go as well. See the discussion here:
>> +// <https://listman.redhat.com/archives/libguestfs/2021-September/msg00101.html>.
>> +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 +182,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)
>>  }
>>  
>>  func count_string_list (argv **C.char) int {
>> @@ -167,11 +200,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))
>>  }
>>  
>>  func return_string_list (argv **C.char) []string {
>> -- 
>> 2.19.1.3.g30247aa5d201
>>
>