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.

Sign up for GitHub

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

Jump to bottom

librados: invalid free() in rados_getxattrs_next() #20260

Merged
merged 2 commits into from Feb 6, 2018
Merged

librados: invalid free() in rados_getxattrs_next() #20260

merged 2 commits into from Feb 6, 2018

Conversation

guzhongyan
Copy link
Contributor

Invalid free() can cause corruption when getting an object
attribute with empty value.

Check the validity of the pointer before free(). Also move
the free() call at the start of rados_getxattrs_next() to
avoid memory leak.

Fixes: http://tracker.ceph.com/issues/22042
Signed-off-by: Gu Zhongyan guzhongyan@360.cn

@tchaikov tchaikov self-requested a review February 2, 2018 14:41
liewegas
liewegas reviewed Feb 2, 2018
View reviewed changes
src/librados/librados.cc Outdated
@@ -4281,14 +4281,17 @@ extern "C" int rados_getxattrs_next(rados_xattrs_iter_t iter,
{
tracepoint(librados, rados_getxattrs_next_enter, iter);
librados::RadosXattrsIter *it = static_cast<librados::RadosXattrsIter*>(iter);
if(it->val) {
Copy link
Member

Choose a reason for hiding this comment

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

s/if(/if (/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review. Done.

liewegas
liewegas approved these changes Feb 2, 2018
View reviewed changes
Copy link
Member

@liewegas liewegas left a comment

Choose a reason for hiding this comment

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

whitespace nit aside, looks good! can you push an update for that?

@liewegas
Copy link
Member

liewegas commented Feb 2, 2018

Also, can you add your attr test case to src/test/pybind/test_rados.py?

@guzhongyan
librados: invalid free() in rados_getxattrs_next()
015736d 
Invalid free() can cause corruption when getting an object
attribute with empty value.

Check the validity of the pointer before free(). Also move
the free() call at the start of rados_getxattrs_next() to
avoid memory leak.

Fixes: http://tracker.ceph.com/issues/22042
Signed-off-by: Gu Zhongyan <guzhongyan@360.cn>
@guzhongyan
test/pybind/test_rados.py: expand test_xattrs/test_obj_xattrs
b79107d 
to cover attribute with empty value case.

Signed-off-by: Gu Zhongyan <guzhongyan@360.cn>
@guzhongyan
Copy link
Contributor Author

guzhongyan commented Feb 5, 2018 •
edited

@liewegas, attr test case is also added.
with the expaned attr test case:
without my fix, the test case would be failed:
[guzhongyan@rg1-ceph10 ~/ceph/build/bin]$ PYTHONPATH=pybind nosetests ../../src/test/pybind/test_rados.py:TestIoctx.test_xattrs
rados_getxattrs
*** Error in `/usr/bin/python': double free or corruption (fasttop): 0x00000000023007e0 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7c619)[0x7fd86a5c0619]
/lib64/librados.so.2(ZNSt6vectorISsSaISsEE12emplace_backIJSsEEERSsDpOT+0x16e)[0x7fd85e7028fe]
/lib64/librados.so.2(rados_mon_command+0x131)[0x7fd85e6f43d1]
/usr/lib64/python2.7/site-packages/rados.so(+0x8eaf9)[0x7fd85ea6baf9]

[guzhongyan@rg1-ceph10 ~/ceph/build/bin]$ PYTHONPATH=pybind nosetests ../../src/test/pybind/test_rados.py:TestIoctx.test_obj_xattrs
rados_getxattrs
*** Error in `/usr/bin/python': double free or corruption (fasttop): 0x0000000002d97800 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7c619)[0x7f4e43354619]
/lib64/librados.so.2(_ZN8librados15RadosXattrsIterD1Ev+0xd)[0x7f4e374b822d]
/lib64/librados.so.2(rados_getxattrs_end+0x24)[0x7f4e374708d4]
/usr/lib64/python2.7/site-packages/rados.so(+0x174e5)[0x7f4e377884e5]
/lib64/libpython2.7.so.1.0(PyEval_EvalFrameEx+0x6f3d)[0x7f4e440a4cfd]

after my fix, things look good.
[guzhongyan@rg1-ceph10 ~/ceph/build/bin]$ PYTHONPATH=pybind nosetests ../../src/test/pybind/test_rados.py:TestIoctx.test_xattrs
rados_getxattrs
.

Ran 1 test in 4.299s

OK

[guzhongyan@rg1-ceph10 ~/ceph/build/bin]$ PYTHONPATH=pybind nosetests ../../src/test/pybind/test_rados.py:TestIoctx.test_obj_xattrs
rados_getxattrs
.

Ran 1 test in 3.633s

OK

@tchaikov
Copy link
Contributor

tchaikov commented Feb 5, 2018

retest this please.

@tchaikov
Copy link
Contributor

tchaikov commented Feb 6, 2018

@tchaikov tchaikov merged commit 221d8a1 into ceph:master Feb 6, 2018
@guzhongyan guzhongyan deleted the fix-22042 branch February 6, 2018 09:36
@gregsfortytwo
Copy link
Member

This may have caused issues in backport testing and it just doesn't seem correct. From #20381:

I dunno what's going on or if it's the cause, but it sure looks like this PR causes us to try to free a random pointer when we reach the end of the list.
(And the rest of the function is a great mess too, using memory after free!)

@tchaikov tchaikov mentioned this pull request Apr 1, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

@liewegas liewegas liewegas approved these changes

Assignees
No one assigned
Labels
bug-fix common needs-qa wip-kefu-testing
Projects
None yet
Milestone
No milestone
4 participants
@guzhongyan @liewegas @tchaikov @gregsfortytwo