gccgo: cgo: false positive cgo argument has Go pointer to Go pointer #23391
Comments
|
Can you show us a way to reproduce the problem locally? |
|
Oddly, I can't reproduce this after rebasing my gccgo work on CockroachDB's latest master branch. Nominally, the repro instructions are roughly the same as #23393 (comment), except an on-disk store must be used ( |
|
Ah, this reproduces occasionally using when |
|
@thanm care to look into this one, since you already have the necessary local setup? |
|
I'll take a look later this morning |
|
Did a bit of digging under gdb: The interesting stuff is happening in frame 6 (Go function passing a pointer) and frames 3 and below (Go functions checking that pointer). So we are passing a pointer to a null pointer. This is weird. We're skipping the dereference So, somehow the
In other words, gccgo is setting |
|
Where you print |
|
|
Thanks. I'm not seeing what is wrong here. It's correct that |
My reading of the code is that it decides that |
|
In this case, |
|
To put it another way, where is the error reported? It's not reported by the code you discussed above. |
|
OK, here's the actual panic backtrace: confirming that the passed pointer points to a null pointer: interesting stuff starts at frame 5: things we care about are arguments to so we don't know the value of interestingly, next frame: so this type is a struct (that's where we are in the type switch). we know this time we land in the so we execute this is what's surpring. at this point, after the dereference, I would expect to end up with the null pointer we started with, but that's obviously not the case. the actual value of let me know if there's more detail I can provide. |
|
I didn't notice that you weren't showing the full function call. Looking at the source code, it's status := C.DBOpen(&r.rdb, goToCSlice([]byte(r.cfg.Dir)),
C.DBOptions{
cache: r.cache.cache,
block_size: C.uint64_t(blockSize),
wal_ttl_seconds: C.uint64_t(walTTL),
logging_enabled: C.bool(log.V(3)),
num_cpu: C.int(runtime.NumCPU()),
max_open_files: C.int(maxOpenFiles),
use_switching_env: C.bool(newVersion == versionCurrent),
must_exist: C.bool(r.cfg.MustExist),
extra_options: goToCSlice(r.cfg.ExtraOptions),
})The type passed to type _Ctype_struct___7 struct {
cache *_Ctype_struct_DBCache
block_size _Ctype_uint64_t
wal_ttl_seconds _Ctype_uint64_t
logging_enabled _Ctype__Bool
_ [3]byte
num_cpu _Ctype_int
max_open_files _Ctype_int
use_switching_env _Ctype__Bool
must_exist _Ctype__Bool
_ [2]byte
extra_options _Ctype_struct___0
}So the problem is not the The stack trace shows it looking at this struct in frame 4, and then looking at another struct in frame 3. That suggests that the second struct is the type _Ctype_struct___0 struct {
data *_Ctype_char
len _Ctype_int
_ [4]byte
}In frame 2 we are looking at a pointer, which in this case must be the Looking back at the Go code, the field is initialized by a call to Can you find out what value is in |
|
It's |
|
Hmmm, OK, clearly there is something wrong with my analysis. There is another pointer in the @thanm Any luck reproducing this locally? |
|
Heh, figured you'd ask for that. |
|
Oh, and the memory at that address is null. |
|
Thanks. I think we need to know whether that value ( |
|
As you say - it is set by a call to C.DBNewCache, which is obviously not a
Go function (it is implemented in c-deps/libroach/cache.cc) with a call to
RocksDB's C++ API.
…On Fri, Feb 2, 2018 at 3:04 PM, Ian Lance Taylor ***@***.***> wrote:
Thanks. I think we need to know whether that value (0x7fffe7c13040) is a
Go pointer (whether cgoIsGoPointer would return true for it) and, if so,
what allocated block it is pointing to. If I'm reading the code correctly,
it is set by a call to C.DBNewCache.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23391 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPN_xOuPBW0ztfhxva1mD-xvFIfAUks5tQ2oygaJpZM4RYcDX>
.
|
|
Thanks, but does |
|
How can I call that function? I don't see a symbol that fits the name, so I
suspect it's all been inlined. Any ideas?
…On Fri, Feb 2, 2018 at 4:04 PM, Ian Lance Taylor ***@***.***> wrote:
Thanks, but does cgoIsGoPointer return true for it? And is that in fact
the value that the runtime package is finding the error for?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23391 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPJ8119QJd7WK6Qc8wM7TbUJG7gA5ks5tQ3hvgaJpZM4RYcDX>
.
|
|
Do you feel like rebuilding libgo without optimization? You should be able to do that by running |
|
Done: but how do I call it? |
|
Ah, just need to stringify it! |
|
Now that nothing is inlined, this is much easier to reason about. so that's not |
|
Here's the cgo stub (reformatted by hand): status := func(_cgo0 **_Ctype_struct_DBEngine, _cgo1 _Ctype_struct___0, _cgo2 _Ctype_struct___7) _Ctype_struct___1 {
_cgoCheckPointer(_cgo0, true)
_cgoCheckPointer(_cgo2)
return (_Cfunc_DBOpen)(_cgo0, _cgo1, _cgo2)
}(&r.rdb, goToCSlice([]byte(r.cfg.Dir)),
_Ctype_struct___7{
cache: r.cache.cache,
block_size: _Ctype_uint64_t(blockSize),
wal_ttl_seconds: _Ctype_uint64_t(walTTL),
logging_enabled: _Ctype__Bool(log.V(3)),
num_cpu: _Ctype_int(runtime.NumCPU()),
max_open_files: _Ctype_int(maxOpenFiles),
use_switching_env: _Ctype__Bool(newVersion == versionCurrent),
must_exist: _Ctype__Bool(r.cfg.MustExist),
extra_options: goToCSlice(r.cfg.ExtraOptions),
})so the slice isn't being checked, but the DBOptions is. Since |
|
Assigning |
|
I probably can't get back to this until Monday. Would you consider doing
this on a call or over Gitter?
…On Feb 2, 2018 18:37, "Ian Lance Taylor" ***@***.***> wrote:
Assigning DBOptions to an interface value, including allocating it, is
normal behavior. We aren't going to check that pointer itself; we're going
to check the value to which it points. Now that you have a version without
optimization, can you find the pointer value that actually causes the
problem? The one passed to cgoCheckUnknownPointer when it panics?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23391 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPAylLSSzoOzA_nQ-9LSG46w62Z4sks5tQ5xRgaJpZM4RYcDX>
.
|
|
Sorry, no, I am continually interrupted. Most effective would be some way that I can reproduce the problem myself. |
the we're inspecting one of the struct's fields, and it has type type _Ctype_struct___0 struct {
data *_Ctype_char
len _Ctype_int
_ [4]byte
}which is also the type of the second argument to We're checking the first field, which is a Now we've deferenced the pointer - so now our and the next frame panics. However, checking this pointer is only valid if the slice is not empty, right? Let's check if it is: Seems like we're passing a nil slice into func goToCSlice(b []byte) C.DBSlice {
if len(b) == 0 {
return C.DBSlice{data: nil, len: 0}
}
return C.DBSlice{
data: (*C.char)(unsafe.Pointer(&b[0])),
len: C.int(len(b)),
}
}and So something is going wrong here - we're passing a This is looking really suspicious. We passed in nil data and a zero length but it came out the other side with length 1. @ianlancetaylor is this enough to go on? |
|
I haven't really looked at the above yet, but I committed a patch on Saturday that could and did lead to corruption of the slice structure in very rare cases. |
|
Could you link to the patch? |
|
The patch is https://gcc.gnu.org/ml/gcc-patches/2018-02/msg00140.html . |
|
Rebuilt and retested - the issue still reproduces. |
Why add Not that that explains anything. Can you go to your frame 7 ( |
|
Added 16 because that's the field's offset - I printed the field out at one
point. I'll try to get you that value in an hour or so.
On Feb 5, 2018 18:28, "Ian Lance Taylor" <[email protected]> wrote:
x 0xc4200c0af0+16
Why add 16? The length is at offset 8, and it's only four bytes long.
Not that that explains anything.
Can you go to your frame 7 (engine.func1) and print the value of cgo2? It
should be the DBOptions struct. If the pointer is nil there we need to find
out where it changes.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23391 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPO8UUHCi-oo2gn0UnhhYbMVG9oF6ks5tR46HgaJpZM4RYcDX>
.
|
|
Scratch that, I won't be able to check until tomorrow.
…On Feb 5, 2018 18:30, "Tamir Duberstein" ***@***.***> wrote:
Added 16 because that's the field's offset - I printed the field out at
one point. I'll try to get you that value in an hour or so.
On Feb 5, 2018 18:28, "Ian Lance Taylor" ***@***.***> wrote:
x 0xc4200c0af0+16
Why add 16? The length is at offset 8, and it's only four bytes long.
Not that that explains anything.
Can you go to your frame 7 (engine.func1) and print the value of cgo2? It
should be the DBOptions struct. If the pointer is nil there we need to
find out where it changes.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#23391 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPO8UUHCi-oo2gn0UnhhYbMVG9oF6ks5tR46HgaJpZM4RYcDX>
.
|
|
@ianlancetaylor |
|
Thanks. I didn't expect And now I see the problem. The value stored in the |
|
Change https://golang.org/cl/92275 mentions this issue: |
|
Sent https://golang.org/cl/92275. Thanks for your patience. |
|
No problem. Thanks for your help and persistence. EDIT: for posterity, the mentioned reflect change appears to be 9bbb07d. |
The offset field in structfield has changed to offsetAnon, and now
requires a shift to get the actual offset value.
Fixes golang/go#23391
Reviewed-on: https://go-review.googlesource.com/92275
git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@257413 138bc75d-0d04-0410-961f-82ee72b054a4
The offset field in structfield has changed to offsetAnon, and now
requires a shift to get the actual offset value.
Fixes golang/go#23391
Reviewed-on: https://go-review.googlesource.com/92275
From-SVN: r257413
What version of Go are you using (
go version)?go version go1.9 gccgo (GCC) 8.0.0 20180108 (experimental) linux/amd64Description
The following code exists in CockroachDB:
https://github.com/cockroachdb/cockroach/blob/4e6ec545cc65dc0051a797996e9642b4b5e8bdc2/pkg/storage/engine/rocksdb.go#L447-L450
https://github.com/cockroachdb/cockroach/blob/4e6ec545cc65dc0051a797996e9642b4b5e8bdc2/pkg/storage/engine/rocksdb.go#L584
https://github.com/cockroachdb/cockroach/blob/4e6ec545cc65dc0051a797996e9642b4b5e8bdc2/c-deps/libroach/db.cc#L1616
Said plainly, initialization across cgo is done by passing a Go
**C.DBEnginewhich points tonilto a C function that allocates and sets the pointed-to*C.DBEngine. This code works perfectly when compiled with gc, but produces this error when compiled with gccgo:According to https://golang.org/cmd/cgo/#hdr-Passing_pointers:
It's not exactly clear if nil Go pointers are permitted, but the gc implementation certainly permits it. Perhaps gccgo is missing a null check?
@ianlancetaylor
The text was updated successfully, but these errors were encountered: