thr3ads.net - Libguestfs - [Libguestfs] [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation [Sep 2021]

If this information is useful, please help other people find it:
Share via:

Laszlo Ersek

2021-Sep-20 05:23 UTC

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

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)
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)
 }
 
 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))
 }
 
 func return_string_list (argv **C.char) []string {
-- 
2.19.1.3.g30247aa5d201

Richard W.M. Jones

2021-Sep-20 10:36 UTC

head link

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

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,
> 
> - 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)
> 
> 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)
>  }
>  
>  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))
>  }
>  
>  func return_string_list (argv **C.char) []string {
I had to do something similar to this in libnbd recently.

Thanks, ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Daniel P. Berrangé

2021-Sep-20 10:37 UTC

head link

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

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.
> 
> - 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 ?


> 
> 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
  }

>  
>  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)) 
}
>  
>  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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Libguestfs - Sep 2021 - [PATCH 4/4] Go bindings: fix "C array of strings" -- char** -- allocation

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

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

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