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

librados: Fix a potential risk of buffer::list::claim_prepend(list& b… #17661

Merged
merged 1 commit into from Sep 25, 2017

Conversation

Projects
None yet
3 participants
@yunfeiguan
Copy link

commented Sep 12, 2017

…l, unsigned int flags)

Last_p should point to the _buffers.begin() when push front a ptr to _buffers,
which make the bufferlist can be full amount copied.

Fixes: http://tracker.ceph.com/issues/21338
Signed-off-by: Guan yunfei yunfei.guan@xtaotech.com

_len += bl._len;
if (!(flags & CLAIM_ALLOW_NONSHAREABLE))
bl.make_shareable();
_buffers.splice(_buffers.begin(), bl._buffers );
bl._len = 0;
bl.last_p = bl.begin();

// 5. update the last_p point to the lasted ptr
last_p = begin();

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 12, 2017

Contributor

i think we just need the step 5 to fix this issue?

and would you please add a test case in src/test/bufferlist.cc?

@yunfeiguan yunfeiguan force-pushed the yunfeiguan:master branch from faf4c5a to 1f7948c Sep 12, 2017

_len += bl._len;
if (!(flags & CLAIM_ALLOW_NONSHAREABLE))
bl.make_shareable();
_buffers.splice(_buffers.begin(), bl._buffers );
bl._len = 0;
bl.last_p = bl.begin();

// 5. update the last_p point to the lasted ptr
//update the last_p point to the lasted ptr

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 18, 2017

Contributor

nit, add a space before update

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

could you squash these three commits into a single one?

bufferlist b3;
b3.append("1234567", 7);

EXPECT_EQ((unsigned)5, b1.length());

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 18, 2017

Contributor

nit,

EXPECT_EQ(5u, b1.length());

src_buf.claim_prepend(b3);
EXPECT_EQ((unsigned)(8+7), src_buf.length());

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 18, 2017

Contributor

remove unnecessary empty lines.


src_buf.copy(0, src_buf.length(), dest_buf);
EXPECT_EQ((unsigned)15, dest_buf.length());

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 18, 2017

Contributor

ditto.


EXPECT_EQ((unsigned)7, dest_buf.front().length());
EXPECT_EQ((unsigned)6, dest_buf.back().length());

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 18, 2017

Contributor

ditto.

@yunfeiguan yunfeiguan force-pushed the yunfeiguan:master branch from 829f2f6 to 758105e Sep 19, 2017

@yunfeiguan

This comment has been minimized.

Copy link
Author

commented Sep 20, 2017

@tchaikov help to review quickly please, thanks.

@@ -1809,6 +1809,9 @@ using namespace ceph;
_buffers.splice(_buffers.begin(), bl._buffers );
bl._len = 0;
bl.last_p = bl.begin();

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 20, 2017

Contributor

nit, remove this empty line please

@tchaikov
Copy link
Contributor

left a comment

aside from the nit, lgtm.

@@ -1809,6 +1809,9 @@ using namespace ceph;
_buffers.splice(_buffers.begin(), bl._buffers );
bl._len = 0;
bl.last_p = bl.begin();

// update the last_p point to the first ptr

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 20, 2017

Contributor

this comment is a translation of following statement. so i don't think it helps. if you really want to document it.

// we modified _buffers

would suffice.

YunfeiGuan
librados: Fix a potential risk of buffer::list::claim_prepend(list& b…
…l, unsigned int flags)

Last_p should point to the _buffers.begin() when push front a ptr to _buffers,
which make the bufferlist can be full amount copied.

Fixes: http://tracker.ceph.com/issues/21338
Signed-off-by: Guan yunfei <yunfei.guan@xtaotech.com>

@yunfeiguan yunfeiguan force-pushed the yunfeiguan:master branch from 758105e to ffa1f99 Sep 20, 2017

@tchaikov tchaikov added the needs-qa label Sep 20, 2017

@liewegas liewegas merged commit a79c78c into ceph:master Sep 25, 2017

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
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.