Skip to content

Commit

Permalink
Fix memory error in devArrMatch (#229)
Browse files Browse the repository at this point in the history 
We recently discovered a memory error in the `devArrMatch()` function: https://gpuci.gpuopenanalytics.com/job/rapidsai/job/gpuci-v0.20/job/cuml/job/prb/job/cuml-gpu-test/CUDA=11.0,GPU_LABEL=gpu,OS=ubuntu16.04,PYTHON=3.7/161/console
```
13:09:34 [----------] 44 tests from FilTests/TreeliteDenseFilTest
13:09:34 [ RUN      ] FilTests/TreeliteDenseFilTest.Import/0
13:09:34 *** Error in `./test/ml': free(): invalid pointer: 0x00007f632b691fe8 ***
13:09:34 ======= Backtrace: =========
13:09:34 /lib/x86_64-linux-gnu/libc.so.6(+0x777f5)[0x7f632b3447f5]
13:09:34 /lib/x86_64-linux-gnu/libc.so.6(+0x8038a)[0x7f632b34d38a]
13:09:34 /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f632b35158c]
13:09:34 /workspace/ci/artifacts/cuml/cpu/conda_work/cpp/build/libcuml++.so(_ZN8treelite9ModelImplIffED0Ev+0xf5)[0x7f632c556405]
```
which was traced to  the `devArrMatch()` function as follows:
```
$ valgrind ./cpp/build/test/ml --gtest_filter=FilTests/TreeliteDenseFilTest.Import/0
==6398== Mismatched free() / delete / delete []
==6398==    at 0x483D1CF: operator delete(void*, unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==6398==    by 0x209287: std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() (in /home/ubuntu/cuml/cpp/build/test/ml)
==6398==    by 0x2AC253: testing::AssertionResult raft::devArrMatch<float, raft::CompareApprox<float> >(float const*, float const*, unsigned long, raft::CompareApprox<float>, CUstream_st*) (in /home/ubuntu/cuml/cpp/build/test/ml)
==6398==    by 0x2AC51C: ML::BaseFilTest::compare() (in /home/ubuntu/cuml/cpp/build/test/ml)
==6398==    by 0x4858098D: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x48580BE0: testing::Test::Run() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x48580F0E: testing::TestInfo::Run() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x48581035: testing::TestSuite::Run() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x485815EB: testing::internal::UnitTestImpl::RunAllTests() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x48581858: testing::UnitTest::Run() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x4853007E: main (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest_main.so)
==6398==  Address 0x232bfa860 is 0 bytes inside a block of size 160,000 alloc'd
==6398==    at 0x483C583: operator new[](unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==6398==    by 0x2ABFF8: testing::AssertionResult raft::devArrMatch<float, raft::CompareApprox<float> >(float const*, float const*, unsigned long, raft::CompareApprox<float>, CUstream_st*) (in /home/ubuntu/cuml/cpp/build/test/ml)
==6398==    by 0x2AC51C: ML::BaseFilTest::compare() (in /home/ubuntu/cuml/cpp/build/test/ml)
==6398==    by 0x4858098D: void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x48580BE0: testing::Test::Run() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x48580F0E: testing::TestInfo::Run() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x48581035: testing::TestSuite::Run() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x485815EB: testing::internal::UnitTestImpl::RunAllTests() (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x48581858: testing::UnitTest::Run() (in/home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest.so)
==6398==    by 0x4853007E: main (in /home/ubuntu/miniconda3/envs/cuml_dev/lib/libgtest_main.so)
```

**Diagnosis**. The `devArrMatch` functions are allocating a temporary buffer using `new[]` and then assigning it to a `shared_ptr<T>`. This is not valid because the destructor of `shared_ptr<T>` will invoke `delete`, not `delete[]`. Calling `delete` with a buffer allocated by `new[]` leads to an undefined behavior. See https://docs.microsoft.com/en-us/cpp/code-quality/c6278?view=msvc-160.

**Proposed fix**. Use `std:unique_ptr<T[]>` instead to store temporary buffers.

Authors:
  - Philip Hyunsu Cho (https://github.com/hcho3)

Approvers:
  - Corey J. Nolet (https://github.com/cjnolet)

URL: #229
  • Loading branch information
@hcho3
hcho3 authored May 14, 2021
1 parent 4273f42 commit c5030a0
Showing 1 changed file with 8 additions and 8 deletions.
16 changes: 8 additions & 8 deletions cpp/test/test_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ template <typename T, typename L>
testing::AssertionResult devArrMatch(const T *expected, const T *actual,
size_t size, L eq_compare,
cudaStream_t stream = 0) {
std::shared_ptr<T> exp_h(new T[size]);
std::shared_ptr<T> act_h(new T[size]);
std::unique_ptr<T[]> exp_h(new T[size]);
std::unique_ptr<T[]> act_h(new T[size]);
raft::update_host<T>(exp_h.get(), expected, size, stream);
raft::update_host<T>(act_h.get(), actual, size, stream);
CUDA_CHECK(cudaStreamSynchronize(stream));
Expand All @@ -96,7 +96,7 @@ testing::AssertionResult devArrMatch(const T *expected, const T *actual,
template <typename T, typename L>
testing::AssertionResult devArrMatch(T expected, const T *actual, size_t size,
L eq_compare, cudaStream_t stream = 0) {
std::shared_ptr<T> act_h(new T[size]);
std::unique_ptr<T[]> act_h(new T[size]);
raft::update_host<T>(act_h.get(), actual, size, stream);
CUDA_CHECK(cudaStreamSynchronize(stream));
for (size_t i(0); i < size; ++i) {
Expand All @@ -114,8 +114,8 @@ testing::AssertionResult devArrMatch(const T *expected, const T *actual,
size_t rows, size_t cols, L eq_compare,
cudaStream_t stream = 0) {
size_t size = rows * cols;
std::shared_ptr<T> exp_h(new T[size]);
std::shared_ptr<T> act_h(new T[size]);
std::unique_ptr<T[]> exp_h(new T[size]);
std::unique_ptr<T[]> act_h(new T[size]);
raft::update_host<T>(exp_h.get(), expected, size, stream);
raft::update_host<T>(act_h.get(), actual, size, stream);
CUDA_CHECK(cudaStreamSynchronize(stream));
Expand All @@ -139,7 +139,7 @@ testing::AssertionResult devArrMatch(T expected, const T *actual, size_t rows,
size_t cols, L eq_compare,
cudaStream_t stream = 0) {
size_t size = rows * cols;
std::shared_ptr<T> act_h(new T[size]);
std::unique_ptr<T[]> act_h(new T[size]);
raft::update_host<T>(act_h.get(), actual, size, stream);
CUDA_CHECK(cudaStreamSynchronize(stream));
for (size_t i(0); i < rows; ++i) {
Expand Down Expand Up @@ -171,7 +171,7 @@ template <typename T, typename L>
testing::AssertionResult devArrMatchHost(const T *expected_h, const T *actual_d,
size_t size, L eq_compare,
cudaStream_t stream = 0) {
std::shared_ptr<T> act_h(new T[size]);
std::unique_ptr<T[]> act_h(new T[size]);
raft::update_host<T>(act_h.get(), actual_d, size, stream);
CUDA_CHECK(cudaStreamSynchronize(stream));
bool ok = true;
Expand Down Expand Up @@ -203,7 +203,7 @@ testing::AssertionResult diagonalMatch(T expected, const T *actual, size_t rows,
size_t cols, L eq_compare,
cudaStream_t stream = 0) {
size_t size = rows * cols;
std::shared_ptr<T> act_h(new T[size]);
std::unique_ptr<T[]> act_h(new T[size]);
raft::update_host<T>(act_h.get(), actual, size, stream);
CUDA_CHECK(cudaStreamSynchronize(stream));
for (size_t i(0); i < rows; ++i) {
Expand Down

0 comments on commit c5030a0

Please sign in to comment.