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

rgw: fix resource leak in rgw_bucket.cc and rgw_user.cc #17249

Closed

Conversation

scienceluo
Copy link
Contributor

@scienceluo scienceluo commented Aug 25, 2017

Fix coverity check:

CID 1416847 (#1 of 1): Resource leak (RESOURCE_LEAK)
4. leaked_storage: Variable info going out of scope leaks the storage it points to.

CID 1416844 (#1 of 1): Resource leak (RESOURCE_LEAK)
4. leaked_storage: Variable info going out of scope leaks the storage it points to.

CID 1416843 (#1 of 1): Resource leak (RESOURCE_LEAK)
4. leaked_storage: Variable info going out of scope leaks the storage it points to.

Signed-off-by: Luo Kexue luo.kexue@zte.com.cn

@scienceluo scienceluo changed the title rgw:Fixes resource leak in rgw_bucket.cc and rgw_user.cc rgw: fix resource leak in rgw_bucket.cc and rgw_user.cc Aug 25, 2017
@joscollin joscollin self-requested a review August 25, 2017 08:41
Copy link
Member

@joscollin joscollin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I have updated the PR URL in Coverity. Please update it by yourself from the next time onwards.

@scienceluo
Copy link
Contributor Author

Ok, I will update the PR URL in Coverity next time, and thank you for your help. @joscollin

@@ -2781,6 +2781,7 @@ class RGWUserMetadataHandler : public RGWMetadataHandler {
int ret = store->list_raw_objects_init(store->get_zone_params().user_uid_pool, marker,
&info->ctx);
if (ret < 0) {
delete info;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, good catch! i'm fine with this fix as is it, but we might consider taking it step further with std::unique_ptr:

auto info = ceph::make_unique<list_keys_info>();

...

*phandle = (void *)info.release();

the only advantage here being exception safety in the event that store->list_raw_objects_init() throws

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbodley You mean we can handle it like this:

auto info = ceph::make_unique<list_keys_info>();
...
int ret = store->list_raw_objects_init(store->get_zone_params().domain_root, marker,
                                       &info->ctx);
if (ret < 0) {
  *phandle = (void *)info.release();
  return ret;
}
*phandle = (void *)info;

So that we can handle store->list_raw_objects_init throws safety.
Did I catch the point?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbodley That's a good point. Make it std::unique_ptr and let it delete itself when it goes out of scope / throws / returns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*phandle = (void *)info.release();

@scienceluo I think this will again leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joscollin So what's the exact way to solve this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joscollin I found some delete case like this in other functions, maybe we can open another PR to fix this like @cbodley and you said. @joscollin @cbodley what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scienceluo Yes, you can do that. But let's confirm this fix first.

Copy link
Contributor

@cbodley cbodley Aug 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @scienceluo @joscollin. looks good, but the comments are a bit redundant

edit: build failure from missing #include "common/backport14.h"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbodley @joscollin Fixed, thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the comments are a bit redundant

@cbodley Yes, I have put the comments only to explain the change to @scienceluo.

Sorry for the late response, I was on leave.

@joscollin joscollin added DNM and removed needs-qa labels Aug 26, 2017
@scienceluo scienceluo force-pushed the wip-luo-rgw-memory-leak-branch branch 3 times, most recently from 2efefb1 to 2cee309 Compare August 28, 2017 14:36
Signed-off-by: Luo Kexue <luo.kexue@zte.com.cn>
@cbodley cbodley added needs-qa and removed DNM labels Aug 28, 2017
@scienceluo
Copy link
Contributor Author

Close for #17353 instead.

@scienceluo scienceluo deleted the wip-luo-rgw-memory-leak-branch branch September 12, 2017 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants