Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/cgo: definition of GoString is incorrect for exported functions #5548

Closed
mirtchovski opened this issue May 23, 2013 · 8 comments

Comments

@mirtchovski
Copy link
Contributor

commented May 23, 2013

$ go version
go version devel +45468e9eae1f Wed May 22 18:51:15 2013 +0800 darwin/amd64

What steps will reproduce the problem?
Following Minux's suggested steps:

$ mkdir -p  /tmp/gopath/src/cgotest
$ cd /tmp/gopath/src/cgotest
$ cat > main.go <<EOF
package main

import (
        "fmt"
        "unsafe"
)

/*
        #include <stdlib.h>
        void my_printf(char* str);
*/
import "C"

//export hello_from_c
func hello_from_c(s string, i int) {
        fmt.Printf("From C: %s, %d\n", s, i)
}

func main() {
        cstr := C.CString("GoTest")
        defer C.free(unsafe.Pointer(cstr))
        C.my_printf(cstr)
}
EOF
$ cat > part.c <<EOF
#include <_cgo_export.h>
#include <stdio.h>
void my_printf(char *s){
        printf("From Go: %s\n", s);

        GoString d;
        d.p = "C Test";
        d.n = 7;

        // printf("C STRING: %s\n", d.p);
        // fflush(stdout);

        hello_from_c(d, 42);
}
EOF
$ GOPATH=/tmp/gopath go install cgotest
$ /tmp/gopath/bin/cgotest


What is the expected output?
From Go: GoTest
From C: C Test, 42

What do you see instead?
http://play.golang.org/p/QsokZKHkRr

Which operating system are you using?
OSX
@mirtchovski

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2013

Comment 1:

from the go-nuts thread on the subject:
if the code is modified to http://play.golang.org/p/J6fUEF4E_9, then the function works
just fine, but if we replace strlen(t) on line 10 with the value it returns, 6, the
resulting program crashes again. putting a panic() inside hello_from_c in main.go gives
this interesting observation:
with strlen(t):
main.hello_from_c(0x40680b5, 0x6, 0x2a)
cgotest/_obj/main.cgo1.go:17 +0x119
with "6" instead:
[fp=0x41d1e30] main.hello_from_c(0x40680bd, 0x7fff00000006, 0x2a)
cgotest/_obj/main.cgo1.go:16 +0xdd
@mirtchovski

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2013

Comment 2:

Ian Lance Taylor:
I see the bug.  _cgo_export.c looks like this:
void hello_from_c(GoString p0, GoInt p1)
{
        struct {
                GoString p0;
                GoInt p1;
        } __attribute__((packed)) a;
        a.p0 = p0;
        a.p1 = p1;
        crosscall2(_cgoexp_44bb5d457081_hello_from_c, &a, 24);
}
_cgo_export.h has this:
typedef struct { char *p; int n; } GoString;
This means that the generated hello_from_c code is going to convert
from GoInt to int when setting the string length of p0.  That will
break if it so happens that the upper 32 bits of the string length are
not already zero.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 23, 2013

Comment 3:

Labels changed: added priority-soon, go1.1.1, removed priority-triage.

Owner changed to @ianlancetaylor.

Status changed to Accepted.

@minux

This comment has been minimized.

Copy link
Member

commented May 23, 2013

Comment 4:

the fix is easy (don't forget to also fix GoSlice), but i wonder
if we can write a good test case?
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 23, 2013

Comment 5:

https://golang.org/cl/9643044

Status changed to Started.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 24, 2013

Comment 6:

This issue was closed by revision 8f7863d.

Status changed to Fixed.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 30, 2013

Comment 7:

Labels changed: removed go1.1.1.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2013

Comment 8:

Issue #5646 has been merged into this issue.

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.