test: Fix meson build and clean up some warnings #595
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WillAyd
force-pushed
the
fix-meson-compilation
branch
2 times, most recently
from
98a2575
to
1f34f09
Compare
|
Not really familiar with the IPC code or Run End Encoding, but looks like the UBSAN running in the meson build picks up that we are calling memcpy with a NULL dst argument somewhere in the stack: ../src/nanoarrow/common/inline_buffer.h:294:3: runtime error: null pointer passed as argument 1, which is declared to never be null
#0 0x7abd950d3ff9 in ArrowBufferAppendFill ../src/nanoarrow/common/inline_buffer.h:294
#1 0x7abd950d9a2b in ArrowIpcEncoderBuildContiguousBodyBufferCallback ../src/nanoarrow/ipc/encoder.c:511
#2 0x7abd950da36b in ArrowIpcEncoderEncodeRecordBatchImpl ../src/nanoarrow/ipc/encoder.c:546
#3 0x7abd950dac34 in ArrowIpcEncoderEncodeRecordBatch ../src/nanoarrow/ipc/encoder.c:590
#4 0x7abd950db1a5 in ArrowIpcEncoderEncodeSimpleRecordBatch ../src/nanoarrow/ipc/encoder.c:622
#5 0x6531f7d04f6d in ArrowTypeParameterizedTestFixture_NanoarrowIpcNanoarrowArrayRoundtrip_Test::TestBody() ../src/nanoarrow/ipc/decoder_test.cc:815
#6 0x6531f7f1e93b in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2612
#7 0x6531f7ef698a in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2648
#8 0x6531f7e3f9b5 in testing::Test::Run() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2687
#9 0x6531f7e43a3e in testing::TestInfo::Run() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2836
#10 0x6531f7e4877b in testing::TestSuite::Run() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:3015
#11 0x6531f7e93e22 in testing::internal::UnitTestImpl::RunAllTests() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:5920
#12 0x6531f7f24673 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2612
#13 0x6531f7efd21a in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:2648
#14 0x6531f7e8676b in testing::UnitTest::Run() ../subprojects/googletest-1.14.0/googletest/src/gtest.cc:5484
#15 0x6531f7f756ca in RUN_ALL_TESTS() ../subprojects/googletest-1.14.0/googletest/include/gtest/gtest.h:2317
#16 0x6531f7f75497 in main ../subprojects/googletest-1.14.0/googletest/src/gtest_main.cc:64
#17 0x7abd91c29d8f in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
#18 0x7abd91c29e3f in __libc_start_main_impl ../csu/libc-start.c:392
#19 0x6531f7ccf924 in _start (/home/willayd/clones/arrow-nanoarrow/builddir/nanoarrow-ipc-decoder-test+0x42f924)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../src/nanoarrow/common/inline_buffer.h:294:3 in |
WillAyd
commented
Aug 28, 2024
| if (size_bytes == 0) { | ||
| return NANOARROW_OK; | ||
| } | ||
|
|
||
| memset(buffer->data + buffer->size_bytes, value, size_bytes); |
There was a problem hiding this comment.
The UB was triggered by the fact that we were making a call of memset(NULL, ..., 0) - even though size_bytes was 0 I don't think prevents UB from occurring when the dst is NULL
There was a problem hiding this comment.
Should there be a test for this case?
There was a problem hiding this comment.
The test suite is what hits this in the first place. Are you asking to split that off into its own dedicated test?
There was a problem hiding this comment.
Maybe just a few lines here to hit that branch explicitly?
arrow-nanoarrow/src/nanoarrow/common/buffer_test.cc
Lines 143 to 147 in cf38896
There was a problem hiding this comment.
Ah thanks - makes a lot of sense
paleolimbot
approved these changes
Aug 29, 2024
| if (size_bytes == 0) { | ||
| return NANOARROW_OK; | ||
| } | ||
|
|
||
| memset(buffer->data + buffer->size_bytes, value, size_bytes); |
There was a problem hiding this comment.
Should there be a test for this case?
WillAyd
force-pushed
the
fix-meson-compilation
branch
from
af0b045
to
5373a14
Compare
paleolimbot
approved these changes
Sep 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The meson build is broken without this dependency. Also fixed up a few warnings around unused variables and the use of uninitialized struct members