Fix ASAN failures. #345
Fix ASAN failures. #345
Conversation
shaunrd0
requested review from
NullHypothesis,
ypatia,
snagles and
thetorpedodog
as code owners
.github/workflows/tiledb-go.yml
Outdated
| @@ -123,7 +123,6 @@ jobs: | |||
|
|
|||
| Linux_Address_Sanitizer: | |||
| # Only run this job for releases | |||
| if: github.event_name == 'release' && github.event.action == 'published' | |||
There was a problem hiding this comment.
Just testing with CI on this PR, once I see it's passing I'll revert this so it's only ran for releases.
There was a problem hiding this comment.
Reverted this, but kept the removal of continue-on-error, there is a full CI run here with passing ASAN jobs
https://github.com/TileDB-Inc/TileDB-Go/actions/runs/10497212377/job/29079527674?pr=345
|
For these lint errors, you should be able to switch them out with a call to the Lines 21 to 24 in 0b1650f |
array.go
Outdated
| return nil, false, err | ||
| } | ||
| // Wrapped in a function so `dimension` will be cleaned up with defer each time the function completes. | ||
| err = func() error { |
There was a problem hiding this comment.
one ever-so-minor nit: I recommend making this err := ..., so that the err here is a new variable independent of the one outside the loop.
There was a problem hiding this comment.
Thanks for working on this! I'm surprised by how many
Freecalls were missing.
I actually didn’t realize that after accessing, for example, someArray.Schema(), you were able/allowed to free the schema that was returned to you—I thought it belonged to the array. (Reading the code itself makes it clear that you can, but the GC can still handle it for you if you don’t care about freeing things ASAP (which arguably we should inside our own library, just for the sake of being responsible).)
array.go
Outdated
| return nil, false, err | ||
| } | ||
| // Wrapped in a function so `dimension` will be cleaned up with defer each time the function completes. | ||
| err = func() error { |
There was a problem hiding this comment.
one ever-so-minor nit: I recommend making this err := ..., so that the err here is a new variable independent of the one outside the loop.
| } | ||
|
|
||
| dataTypeSize := dimType.Size() | ||
| err = func() error { |
query_test.go
Outdated
| @@ -2986,8 +2984,7 @@ func TestGetDataBuffer(t *testing.T) { | |||
| require.NoError(t, err) | |||
| storedDataBuffer, ok := storedBuffer.([]int32) | |||
| require.True(t, ok) | |||
| require.Equal(t, ((*reflect.SliceHeader)(unsafe.Pointer(&dataBuffer)).Data), | |||
| ((*reflect.SliceHeader)(unsafe.Pointer(&storedDataBuffer)).Data)) | |||
| require.Equal(t, uintptr(slicePtr(dataBuffer)), uintptr(slicePtr(storedDataBuffer))) | |||
There was a problem hiding this comment.
you should be able to remove these uintptr conversions as well, since both of them are now unsafe.Pointers (I think?)
| @@ -383,7 +383,7 @@ func (v *VFS) Read(fh *VFSfh, offset uint64, nbytes uint64) ([]byte, error) { | |||
| // appends data at the end of the file. If the file does not exist, | |||
| // it will be created. | |||
| func (v *VFS) Write(fh *VFSfh, bytes []byte) error { | |||
| cbuffer := C.CBytes(bytes) | |||
| cbuffer := slicePtr(bytes) | |||
There was a problem hiding this comment.
This looks like a very, very good change: C.CBytes makes a copy of the []byte it’s given, which meant that every time we were calling someVFS.Write(fh, ourData), we were leaking a copy of our data. Since this is now using slicePtr, we’re giving it the address of our existing []byte and telling core to look in there.
There was a problem hiding this comment.
…although I think we may also want a defer runtime.KeepAlive(bytes) right after this, because otherwise bytes is never referenced once it enters the C.tiledb_vfs_write call and thus would be eligible for GC as it is in progress.
shaunrd0
force-pushed
the
smr/asan-failures
branch
from
d9c9ec0
to
5afa0cd
Compare
shaunrd0
force-pushed
the
smr/asan-failures
branch
from
5afa0cd
to
fefe73d
Compare
This fixes ASAN failures seen on CI, it looks like they were due to missing calls to
Free()either in the example code or internal definitions.If this looks correct I can follow up and look for similar errors. At the moment this only fixes those reported by
build_with_sanitizer_and_run.shso ASAN CI should pass now, but I would expect there's other similar errors that are not covered by the examples.