Re: [RESEND PATCH 03/12] golang/xenlight: fix string conversion in generated toC functions

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

On Thu, Jul 01, 2021 at 02:09:47PM +0000, George Dunlap wrote:
> 
> 
> > On Jun 21, 2021, at 5:11 PM, Nick Rosbrook <rosbrookn@xxxxxxxxx> wrote:
> > 
> > On Fri, Jun 18, 2021 at 11:00:26AM +0000, George Dunlap wrote:
> >> 
> >> 
> >>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@xxxxxxxxx> wrote:
> >>> 
> >>> In gengotypes.py, the toC functions only set C string fields when
> >>> the Go strings are non-empty. However, to prevent segfaults in some
> >>> cases, these fields should always at least be set to nil so that the C
> >>> memory is zeroed out.
> >>> 
> >>> Update gengotypes.py so that the generated code always sets these fields
> >>> to nil first, and then proceeds to check if the Go string is non-empty.
> >>> And, commit the new generated code.
> >>> 
> >>> Signed-off-by: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> >> 
> >> So wait — if you do
> >> 
> >> var foo C.typename
> >> 
> >> Then golang won’t automatically zero out `foo`?
> >> 
> >> That seems like a bug really; but assuming this fixes real behavior you’ve 
> >> encountered:
> > 
> > I would have to dig in again to figure out exactly what Go/cgo is doing
> > here, and whether or not this is a bug. But, the behavior I observed was
> > that without these nil assignments, I would sometimes get segfaults in
> > libxl_string_copy. This patch ensures that libxl__str_dup is not called
> > in the empty string case, thus avoiding the segfault.
> 
> I skimmed through the CGo page again when I was looking at this, and didn’t 
> see anything specified about what happens if something is passed to a C 
> function before being used by golang.  If you get a chance, I think it would 
> be good to try to file a ticket with the golang project, pointing out the 
> observed behavior, and asking them to either:
> 
> 1. Document that the golang compiler may not initialize a structure before 
> passing it in to a C function
> 
> 2. Document that it *will* initialize values to zero, and fix the bug.
> 
Sorry for the late reply. But that's a good idea, I can try and come up
with a reproducible example and open an issue.

Thanks,
NR