Re: [Xen-devel] [PATCH v4 01/14] golang/xenlight: Create stub package

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On Thu, Mar 16, 2017 at 7:08 PM, Ronald Rojas <ronladred@xxxxxxxxx> wrote:
> Create a basic Makefile to build and install libxenlight Golang
> bindings. Also add a stub package which only opens libxl context.
>
> Include a global xenlight.Ctx variable which can be used as the
> default context by the entire program if desired.
>
> For now, return simple errors. Proper error handling will be
> added in next patch.
>
> Signed-off-by: Ronald Rojas <ronladred@xxxxxxxxx>
> Signed-off-by: George Dunlap <george.dunlap@xxxxxxxxxx>

Almost there.  One comment on one of the last changes...

> +/*
> + * Types: Builtins
> + */
> +type Context struct {
> +       ctx *C.libxl_ctx
> +}

The way the libxl interface here is designed, in theory a single
program could have multiple instances of libxl_ctx open at the same
time; this interface follows that lead, in the sense that you could
certainly write something like this:

var contexts [10]xenlight.Ctx

for i := range contexts {
  err = context[i].Open
  ...
}

[do stuff]

for i := range context {
  err = context[i].Close()
  ...
}

Unfortunately...

> +
> +/*
> + * Context
> + */
> +var Ctx Context
> +
> +var logger *C.xentoollog_logger_stdiostream
> +
> +func (Ctx *Context) IsOpen() bool {
> +       return Ctx.ctx != nil
> +}
> +
> +func (Ctx *Context) Open() (err error) {
> +       if Ctx.ctx != nil {
> +               return
> +       }
> +
> +       logger = C.xtl_createlogger_stdiostream(C.stderr, C.XTL_ERROR, 0)

...this means that in the above code you'd leak 9 of the 10 loggers, and...

> +       ret := C.libxl_ctx_alloc(&Ctx.ctx, C.LIBXL_VERSION,
> +               0, unsafe.Pointer(logger))
> +
> +       if ret != 0 {
> +               err = fmt.Errorf("Error: %d", -ret)
> +       }
> +       return
> +}
> +
> +func (Ctx *Context) Close() (err error) {
> +       ret := C.libxl_ctx_free(Ctx.ctx)
> +       Ctx.ctx = nil
> +
> +       if ret != 0 {
> +               err = fmt.Errorf("Error: %d", -ret)
> +       }
> +       C.xtl_logger_destroy(unsafe.Pointer(logger))

...at this point you'd repeatedly call destroy() on that 9th pointer
value 10 times, which usually leads to not very entertaining results.

I think we want xenlight.Ctx to contain all the information needed to
open and close it; as long as we're creating a logger automatically,
we should store the pointer to the logger in the Ctx struct.

We should also set the pointer to nil after calling
xtl_logger_destroy() to prevent use-after-free bugs.

(Yay lack of garbage collection again.)

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel