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

cgo: program crashes when more that one package uses cgo #8948

Closed
alexbrainman opened this issue Oct 17, 2014 · 14 comments
Closed

cgo: program crashes when more that one package uses cgo #8948

alexbrainman opened this issue Oct 17, 2014 · 14 comments
Milestone

Comments

@alexbrainman
Copy link
Member

Build this program:

package main

import (
    "database/sql"
    "fmt"
    _ "github.com/mattn/go-sqlite3"
)

func main() {
    db, err := sql.Open("sqlite3", "./test.db")
    if err != nil {
        fmt.Println("err%+v", err)
        return
    }
    defer db.Close()

    err = db.Ping()
    if err != nil {
        panic(err.Error())
    }
    fmt.Println("ok")
}

with "go build -race ..." command on windows, and run the program. Here is the
output:

C:\go\path\src\t>foo.exe
Exception 0xc0000005 0x8 0x6c3180 0x6c3180
PC=0x6c3180
signal arrived during cgo execution

github.com/mattn/go-sqlite3._Cfunc__sqlite3_open_v2(0x1337f70, 0xc08201fbd0, 0x10006,
0x0, 0x0)
        github.com/mattn/go-sqlite3/_obj/_cgo_gotypes.go:102 +0x78
github.com/mattn/go-sqlite3.(*SQLiteDriver).Open(0xc082004620, 0x5fead0, 0x9, 0x0, 0x0,
0x0, 0x0)
        c:/go/path/src/github.com/mattn/go-sqlite3/sqlite3.go:264 +0x238
database/sql.(*DB).conn(0xc08204e000, 0x401217, 0x0, 0x0)
        c:/go/root/src/database/sql/sql.go:659 +0x7df
database/sql.(*DB).Ping(0xc08204e000, 0x0, 0x0)
        c:/go/root/src/database/sql/sql.go:457 +0x4f
main.main()
        C:/go/path/src/t/foo.go:17 +0x230

goroutine 2 [runnable]:
runtime.forcegchelper()
        c:/go/root/src/runtime/proc.go:90
runtime.goexit()
        c:/go/root/src/runtime/proc.c:1651

goroutine 3 [runnable]:
runtime.bgsweep()
        c:/go/root/src/runtime/mgc0.go:66
runtime.goexit()
        c:/go/root/src/runtime/proc.c:1651

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
        c:/go/root/src/runtime/proc.c:1651

goroutine 4 [runnable]:
runtime.runfinq()
        c:/go/root/src/runtime/malloc.go:705
runtime.goexit()
        c:/go/root/src/runtime/proc.c:1651

goroutine 5 [runnable]:
database/sql.(*DB).connectionOpener(0xc08204e000)
        c:/go/root/src/database/sql/sql.go:583
created by database/sql.Open
        c:/go/root/src/database/sql/sql.go:447 +0x41a
rax     0x0
rbx     0x7
rcx     0x6ce4c0
rdx     0x6ce4c0
rdi     0x6ce4f0
rsi     0x6d2ec0
rbp     0x6ce7c0
rsp     0x22fd48
r8      0x2
r9      0x84
r10     0x42
r11     0x1330158
r12     0xc08201faf0
r13     0x1084d60
r14     0xc082016000
r15     0xc082008100
rip     0x6c3180
rflags  0x10246
cs      0x33
fs      0x53
gs      0x2b

Perhaps the problem is in github.com/mattn/go-sqlite3, but it is worth checking.

Alex
@alexbrainman
Copy link
Member Author

@minux
Copy link
Member

minux commented Dec 5, 2014

Comment 2:

Perhaps the race detector shouldn't instrument the cgo wrapper code.

Labels changed: added release-go1.4.

@dvyukov
Copy link
Member

dvyukov commented Dec 5, 2014

Comment 3:

The crash happens inside of C code in
func _Cfunc__sqlite3_open_v2(p0 *_Ctype_char, p1 **_Ctype_struct_sqlite3, p2 _Ctype_int,
p3 *_Ctype_char) (r1 _Ctype_int) {
    _cgo_runtime_cgocall_errno(_cgo_edfc822ad612_Cfunc__sqlite3_open_v2, uintptr(unsafe.Pointer(&p0)))
    return
}
So I don't think that not instrumenting cgo wrapper code will help.
Race detector does not instrument C code, so it seems that the C code just crashes.
What's at PC 0x6c3180? That PC seems to dereference a NULL pointer.

@alexbrainman
Copy link
Member Author

https://code.google.com/p/go/issues/detail?id=8948#c4 comment is lost. It says:

It is crashing in sqlite3_os_init from sqlite3.c trying to execute osGetSystemInfo. I suspect address of real syscall is not setup properly during -race. It works fine when no -race. It might be go linker or gcc fault for all I know. I don't know enough about these things ...

Alex

@ricsmania
Copy link

This also happens with github.com/mattn/go-oci8 when I db.Exec an insert, however I don't get the call stack from inside go-oci8.

It works fine without -race. Queries work either way.

Using Go 1.4.1 x64 on Windows 7

create table test
(
  pk NUMBER,
  id NUMBER not null
)
package main

import (
    "database/sql"
    "fmt"
    _ "github.com/mattn/go-oci8"
)

func main() {
    db, err := sql.Open("oci8", "system/[email protected]:1521/orcl")
    if err != nil {
        fmt.Println(err)
    }

    db.Exec("delete from test")
    _, err = db.Exec("insert into test values (:pk, :id)", 1, 1)
    if err != nil {
        fmt.Println(err)
    }

}
go run -race oracle-insert.go
Exception 0xc0000005 0x8 0xd00000000 0xd00000000
PC=0xd00000000


goroutine 17 [syscall, locked to thread]:
runtime.goexit()
        c:/go/src/runtime/asm_amd64.s:2232 +0x1

goroutine 5 [chan receive]:
database/sql.(*DB).connectionOpener(0xc082076000)
        c:/go/src/database/sql/sql.go:589 +0x74
created by database/sql.Open
        c:/go/src/database/sql/sql.go:452 +0x41a
rax     0x56000000000
rbx     0x0
rcx     0x10
rdx     0x2059e30
rdi     0xc082006380
rsi     0xc082073968
rbp     0x0
rsp     0xc082073a88
r8      0x3a2230
r9      0x0
r10     0x11768e0
r11     0x12ffa08
r12     0xc082073a28
r13     0x1176ca0
r14     0xc08201e000
r15     0x0
rip     0xd00000000
rflags  0xd00010202
cs      0x33
fs      0x53
gs      0x2b
exit status 2

@dvyukov
Copy link
Member

dvyukov commented Feb 16, 2015

If I change the program as (add usage of cgo):

package main

/*
#include <stdio.h>
#include <windows.h>

void foo() {
    SYSTEM_INFO info;
    GetSystemInfo(&info);
    printf("%d\n", info.dwPageSize);
}
*/
import "C"

import (
    "database/sql"
    "fmt"
    _ "github.com/mattn/go-sqlite3"
)

func main() {
    db, err := sql.Open("sqlite3", "./test.db")
    if err != nil {
        fmt.Println("err%+v", err)
        return
    }
    defer db.Close()

    err = db.Ping()
    if err != nil {
        panic(err.Error())
    }
    fmt.Println("ok")
}

Then the program fails even without race detector.

In both working and non-working case, address of GetSystemInfo looks like 0x66b070, which means that it does not come from a windows system library (probably from libmingw?). However, in non-working cases the address of GetSystemInfo seems to be a little off. I can speculate that it is off by size of an object file or or a section in one of the packages. Probably we compute symbol offsets assuming that there is only one package that uses native code...

@dvyukov dvyukov added this to the Go1.5 milestone Feb 16, 2015
@dvyukov dvyukov changed the title runtime: go build -race problem with sqlite3 on windows cgo: program crashes when more that one package uses cgo Feb 16, 2015
@alexbrainman
Copy link
Member Author

@dvyukov,

I agree with you. This looks like effect of using 2 different packages (each must use cgo) in one program. When Go linker produces final executable, it might use different relocation strategies for external Windows API functions (like for example GetLastError). If single executable contains 2 function references that require different strategies, the linker just pick one at random. The relocations would be wrong for some function references, and, when the function is called, it just crashes. Running this test:

diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go
index 7152b93..b5ff65e 100644
--- a/src/runtime/crash_cgo_test.go
+++ b/src/runtime/crash_cgo_test.go
@@ -80,6 +80,17 @@ func TestCgoExternalThreadSIGPROF(t *testing.T) {
    }
 }

+func TestCgoDLLImports(t *testing.T) {
+   if runtime.GOOS != "windows" {
+       t.Skip("skipping windows specific test")
+   }
+   got := executeTest(t, cgoDLLImportsMainSource, nil, "a/a.go", cgoDLLImportsPkgSource)
+   want := "OK\n"
+   if got != want {
+       t.Fatalf("expected %q, but got %v", want, got)
+   }
+}
+
 const cgoSignalDeadlockSource = `
 package main

@@ -267,3 +278,43 @@ func main() {
    println("OK")
 }
 `
+
+const cgoDLLImportsMainSource = `
+package main
+
+/*
+#include <windows.h>
+
+DWORD getthread() {
+   return GetCurrentThreadId();
+}
+*/
+import "C"
+
+import "./a"
+
+func main() {
+   C.getthread()
+   a.GetThread()
+   println("OK")
+}
+`
+
+const cgoDLLImportsPkgSource = `
+package a
+
+/*
+#cgo CFLAGS: -mnop-fun-dllimport
+
+#include <windows.h>
+
+DWORD agetthread() {
+   return GetCurrentThreadId();
+}
+*/
+import "C"
+
+func GetThread() uint32 {
+   return uint32(C.agetthread())
+}
+`
diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go
index 43cea90..dceb7a6 100644
--- a/src/runtime/crash_test.go
+++ b/src/runtime/crash_test.go
@@ -64,7 +64,14 @@ func executeTest(t *testing.T, templ string, data interface{}, extra ...string)
    }

    for i := 0; i < len(extra); i += 2 {
-       if err := ioutil.WriteFile(filepath.Join(dir, extra[i]), []byte(extra[i+1]), 0666); err != nil {
+       fname := extra[i]
+       contents := extra[i+1]
+       if d, _ := filepath.Split(fname); d != "" {
+           if err := os.Mkdir(filepath.Join(dir, d), 0755); err != nil {
+               t.Fatal(err)
+           }
+       }
+       if err := ioutil.WriteFile(filepath.Join(dir, fname), []byte(contents), 0666); err != nil {
            t.Fatal(err)
        }
    }

produces this output

c:\go\root\src>go test -run=TestCgoDLLImports runtime
--- FAIL: TestCgoDLLImports (12.84s)
        crash_cgo_test.go:90: expected "OK\n", but got Exception 0xc0000005 0x8 0x4b9040 0x4b9040
                PC=0x4b9040
                signal arrived during cgo execution

                _/C_/Users/brainman/AppData/Local/Temp/go-build072577395/a._Cfunc_agetthread(0xc000000000)
                        _/C_/Users/brainman/AppData/Local/Temp/go-build072577395/a/_obj/_cgo_gotypes.go:37 +0x4b
                _/C_/Users/brainman/AppData/Local/Temp/go-build072577395/a.GetThread(0xc0000015cc)
                        C:/Users/brainman/AppData/Local/Temp/go-build072577395/a/a.go:16 +0x26
                main.main()
                        C:/Users/brainman/AppData/Local/Temp/go-build072577395/main.go:17 +0x2b

                goroutine 17 [syscall, locked to thread]:
                runtime.goexit()
                        c:/go/root/src/runtime/asm_amd64.s:2446 +0x1
                rax     0xc08202c000
                rbx     0xc08202bf78
                rcx     0x4dcfa0
                rdi     0xc08202c000
                rsi     0x4ba700
                rbp     0x0
                rsp     0x22fe68
                r8      0xc082016000
                r9      0xc08202bf20
                r10     0x0
                r11     0x246
                r12     0x33
                r13     0x1
                r14     0x0
                r15     0x0
                rip     0x4b9040
                rflags  0x10202
                cs      0x33
                fs      0x53
                gs      0x2b
FAIL
FAIL    runtime 13.349s

c:\go\root\src>gcc --version
gcc (GCC) 4.9.1
Copyright (C) 2014 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I had to use "#cgo CFLAGS: -mnop-fun-dllimport" to force the crash with my gcc. But I suspect there are different ways to do it. github.com/mattn/go-sqlite3 does it too.

The code that does relocations is in cmd/ld/data.c:^dynrelocsym. The logic that decides what to do is in cmd/ld/ldpe.c. I discovered all this while investigating issue #9356. I suspect it is a dup.

I don't have fix for this. I tried some ideas but none of them work - I don't know this area enough. Perhaps if someone suggest something, I will try it.

Alex

@dvyukov
Copy link
Member

dvyukov commented Feb 17, 2015

@alexbrainman I have no ideas.

Why do you say that GetCurrentThreadId requires two different relocation strategies? Both symbol references end up in the same module, so I would assume that they require relocation of the same type but maybe with different offsets or something.

@alexbrainman
Copy link
Member Author

... Why do you say that GetCurrentThreadId requires two different relocation strategies?...

I am not saying it requires, I am saying Go linker does that at this moment. If you look in cmd/ld/data.c:^dynrelocsym, you will see that we add JMP instruction for "dynamic" symbols where sym->plt == -2 (like GetCurrentThreadId) but only if sym->got != -2. Sometimes (in some imported packages) GetCurrentThreadId has sym->got != -2, and sometimes sym->got == -2 (see in cmd/ld/ldpe.c). So if you use both of these packages at the same time, you end up generating JMP when you should not or vice versa.

Alex

@dvyukov
Copy link
Member

dvyukov commented Feb 17, 2015

I see.
I can't say that I fully understand that code, but it seems to me that we choose the strategy for every reference to a symbol (loop over s->r). That is, one reference can have got type and another plt type, and so we will emit both.
I am not sure whether it is necessary to emit several GOT entries for a single symbol. Perhaps a single entry per symbol will do. But that's a different story.

@alexbrainman
Copy link
Member Author

Maybe this https://go-review.googlesource.com/5711 will fix it.

Alex

@mattn
Copy link
Member

mattn commented Feb 24, 2015

Okay, I'll try this.

@minux
Copy link
Member

minux commented Mar 24, 2015

Should be fixed by external linking support, #4069.

@minux minux closed this as completed Mar 24, 2015
alexbrainman added a commit that referenced this issue Mar 24, 2015
The test is a simple reproduction of issue 9356.

Update #8948.
Update #9356.

Change-Id: Ia77bc36d12ed0c3c4a8b1214cade8be181c9ad55
Reviewed-on: https://go-review.googlesource.com/7618
Reviewed-by: Minux Ma <[email protected]>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/5711 mentions this issue.

@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants