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

test/librados: Silence Coverity memory leak warnings #12442

Merged
merged 2 commits into from Jan 4, 2017

Conversation

Projects
None yet
3 participants
@badone
Copy link
Contributor

badone commented Dec 12, 2016

Use smart pointers to silence Coverity warnings about leaked resources.

Signed-off-by: Brad Hubbard bhubbard@redhat.com

@badone badone force-pushed the badone:wip-coverity-librados-TestCase branch from fb88a38 to 398fea5 Dec 12, 2016

@@ -343,8 +344,8 @@ void RadosTestPP::cleanup_namespace(librados::IoCtx ioctx, std::string ns)
ioctx.locator_set_key(it->get_locator());
ObjectWriteOperation op;
op.remove();
librados::AioCompletion *completion = s_cluster.aio_create_completion();
ASSERT_EQ(0, ioctx.aio_operate(it->get_oid(), completion, &op,
std::unique_ptr<librados::AioCompletion> completion(s_cluster.aio_create_completion());

This comment has been minimized.

Copy link
@tchaikov

tchaikov Dec 12, 2016

Contributor

completion is a ref-counted object. if the completion->release() call in this function fails release and free it, it's a bug.

but i just debugged this method, the completion is released and then deleted as expected.

This comment has been minimized.

Copy link
@badone

badone Dec 12, 2016

Author Contributor

We don't get to the release call if we fail the ASSERT_EQ call. See Coverity issues 1395734 and 1395735. Is there a reason not to use smart pointers here?

This comment has been minimized.

Copy link
@badone

badone Dec 12, 2016

Author Contributor

Ok, I see the put_unlock call. What about if we do something like the following?

completion.release()->release();

This comment has been minimized.

Copy link
@tchaikov

tchaikov Dec 13, 2016

Contributor

a single release() is good enough, and we already have it. what we are missing is the error handling of ASSERT_EQ(), i'd suggest use a unique_ptr<> or intrusive_ptr<> here if we want to have ASSERT_EQ() in TearDown().

This comment has been minimized.

Copy link
@badone

badone Dec 13, 2016

Author Contributor

The first release releases the pointer from the smart pointer, the second is the original completion->release() call. They are not the same call.

ASSERT_EQ(0, r);
rados_ioctx_locator_set_key(ioctx, key);
ASSERT_EQ(0, rados_remove(ioctx, entry));
}
rados_nobjects_list_close(list_ctx);
rados_nobjects_list_close(list_ctx.get());

This comment has been minimized.

Copy link
@tchaikov

tchaikov Dec 12, 2016

Contributor

rados_nobjects_list_close() actually deletes the list_ctx. if coverity complains at this, we might need to ignore this warning or disable it (if possible).

This comment has been minimized.

Copy link
@badone

badone Dec 12, 2016

Author Contributor

I believe Coverity is complaining that the object leaks when we fail the ASSERT_EQ test. See

This comment has been minimized.

Copy link
@tchaikov

tchaikov Dec 13, 2016

Contributor

@badone if we use the plain unique_ptr<> here, we could run into double-free problem. probably, an alternative is to use a unique_ptr<> with a customized deleter, or to use an intrusive_ptr<>? or better off, we should not use ASSERT_* macros in setup/teardown, as they are not part of test by themselves. but i see this would be a more involved change, then.

@tchaikov tchaikov removed the needs-review label Dec 12, 2016

@tchaikov tchaikov self-assigned this Dec 13, 2016

ASSERT_EQ(0, r);
rados_ioctx_locator_set_key(ioctx, key);
ASSERT_EQ(0, rados_remove(ioctx, entry));
}
rados_nobjects_list_close(list_ctx);
rados_nobjects_list_close(list_ctx.get());

This comment has been minimized.

Copy link
@tchaikov

tchaikov Dec 13, 2016

Contributor

@badone if we use the plain unique_ptr<> here, we could run into double-free problem. probably, an alternative is to use a unique_ptr<> with a customized deleter, or to use an intrusive_ptr<>? or better off, we should not use ASSERT_* macros in setup/teardown, as they are not part of test by themselves. but i see this would be a more involved change, then.

@@ -343,8 +344,8 @@ void RadosTestPP::cleanup_namespace(librados::IoCtx ioctx, std::string ns)
ioctx.locator_set_key(it->get_locator());
ObjectWriteOperation op;
op.remove();
librados::AioCompletion *completion = s_cluster.aio_create_completion();
ASSERT_EQ(0, ioctx.aio_operate(it->get_oid(), completion, &op,
std::unique_ptr<librados::AioCompletion> completion(s_cluster.aio_create_completion());

This comment has been minimized.

Copy link
@tchaikov

tchaikov Dec 13, 2016

Contributor

a single release() is good enough, and we already have it. what we are missing is the error handling of ASSERT_EQ(), i'd suggest use a unique_ptr<> or intrusive_ptr<> here if we want to have ASSERT_EQ() in TearDown().

@badone badone changed the title test/librados: Silence Coverity memory leak warnings [DNM] test/librados: Silence Coverity memory leak warnings Dec 14, 2016

@badone

This comment has been minimized.

Copy link
Contributor Author

badone commented Dec 14, 2016

DNM. Working on this after conversation with tchaikov

athanatos and others added some commits Dec 20, 2016

src/include: add scope_guard.h
Signed-off-by: Samuel Just <sjust@redhat.com>
test/librados: Silence Coverity memory leak warnings
Use Sam's scope_guard solution to stop warnings about leaked resources.

Signed-off-by: Brad Hubbard <bhubbard@redhat.com>

@badone badone force-pushed the badone:wip-coverity-librados-TestCase branch from 398fea5 to 7599f0d Dec 21, 2016

@badone

This comment has been minimized.

Copy link
Contributor Author

badone commented Dec 21, 2016

I talked to @athanatos about this and he provided a very elegant solution which I've leveraged to make sure we run the clean up functions regardless of how we exit the scope.

@badone badone changed the title [DNM] test/librados: Silence Coverity memory leak warnings test/librados: Silence Coverity memory leak warnings Dec 21, 2016

@badone badone added the needs-review label Dec 21, 2016

@athanatos

This comment has been minimized.

Copy link
Contributor

athanatos commented Dec 21, 2016

LGTM!

@badone

This comment has been minimized.

Copy link
Contributor Author

badone commented Dec 27, 2016

@tchaikov could you check this again?

scope_guard & operator=(const scope_guard &) = delete;
scope_guard & operator=(scope_guard &&) = default;
scope_guard(F &&f) : f(std::move(f)) {}
~scope_guard() {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Dec 27, 2016

Contributor

@badone why not use boost::BOOST_SCOPE_EXIT or boost::BOOST_SCOPE_EXIT_ALL ?

This comment has been minimized.

Copy link
@badone

badone Dec 27, 2016

Author Contributor

Discussed it with @athanatos and IIRC he had concerns over exception safety and came up with scope_guard

This comment has been minimized.

Copy link
@tchaikov

tchaikov Dec 27, 2016

Contributor

@athanatos could you elaborate a little bit on your concerns about the exception safety? the scope_guard looks good to me and it's way more simpler than the expanded form of its boost alternatives.

i just want to understand if we need to switch over to our home-brew scope_guard at other places where we are using boost::BOOST_SCOPE_EXIT or boost::BOOST_SCOPE_EXIT_ALL because of your concerns.

This comment has been minimized.

Copy link
@athanatos

athanatos Jan 3, 2017

Contributor

I don't think I had an issue with exception safety, but I thought it would be tidier to use lambdas than to use macro magic.

the new change addressed the comments.

@tchaikov tchaikov merged commit 2d51569 into ceph:master Jan 4, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@tchaikov tchaikov removed the needs-review label Jan 4, 2017

@badone badone deleted the badone:wip-coverity-librados-TestCase branch Jan 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.