You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Kenneth Giusti <kg...@apache.org> on 2011/04/26 22:08:35 UTC
Review Request: Reserve header bytes for RDMA send buffers.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/667/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher, Gordon Sim, and Chug Rolke.
Summary
-------
Prevents buffer overflow bug by explicitly allowing RdmaIO layer to reserve header space in send buffers.
This addresses bug QPID-3227.
https://issues.apache.org/jira/browse/QPID-3227
Diffs
-----
/trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp 1096872
/trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1096872
/trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1096872
Diff: https://reviews.apache.org/r/667/diff
Testing
-------
unit tests & scale testing (by hand using perftest).
Thanks,
Kenneth
Re: Review Request: Reserve header bytes for RDMA send buffers.
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/667/#review562
-----------------------------------------------------------
Ship it!
Fixes the bug and as a bonus clearly exposes the usage of frame header bytes.
- Chug
On 2011-04-26 20:08:35, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/667/
> -----------------------------------------------------------
>
> (Updated 2011-04-26 20:08:35)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Chug Rolke.
>
>
> Summary
> -------
>
> Prevents buffer overflow bug by explicitly allowing RdmaIO layer to reserve header space in send buffers.
>
>
> This addresses bug QPID-3227.
> https://issues.apache.org/jira/browse/QPID-3227
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1096872
>
> Diff: https://reviews.apache.org/r/667/diff
>
>
> Testing
> -------
>
> unit tests & scale testing (by hand using perftest).
>
>
> Thanks,
>
> Kenneth
>
>
Re: Review Request: Reserve header bytes for RDMA send buffers.
Posted by Andrew Stitcher <as...@apache.org>.
> On 2011-04-27 15:26:23, Andrew Stitcher wrote:
> > I recommend adding in this extra assertion to RdmaIO.cpp:
> > This would have caught the original bug.
> >
> > --- a/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
> > +++ b/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
> > @@ -213,6 +213,7 @@ namespace Rdma {
> > Buffer* ob = buff ? buff : getSendBuffer();
> > // Add FrameHeader after frame data
> > FrameHeader header(credit);
> > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount())
> > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize);
> > ob->dataCount(ob->dataCount()+FrameHeaderSize);
> > qp->postSend(ob);
> >
> >
>
> Andrew Stitcher wrote:
> Ken pointed out that this assertion would only be good for the old code and the new code needs to add in the reserved space too.
>
> Kenneth Giusti wrote:
> How about something like this (totally untested):
>
> Index: cpp/src/qpid/sys/rdma/rdma_wrap.h
> ===================================================================
> --- cpp/src/qpid/sys/rdma/rdma_wrap.h (revision 1097599)
> +++ cpp/src/qpid/sys/rdma/rdma_wrap.h (working copy)
> @@ -77,6 +77,8 @@
> }
>
> inline void Buffer::dataCount(int32_t s) {
> + // catch any attempt to overflow a buffer
> + assert(s <= bufferSize + reserved);
> sge.length = s;
> }
>
>
> This stashes the check in the buffer code itself at the point where the application sets the length of the output data.
I think we could have both checks in there,
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/667/#review577
-----------------------------------------------------------
On 2011-04-26 20:08:35, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/667/
> -----------------------------------------------------------
>
> (Updated 2011-04-26 20:08:35)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Chug Rolke.
>
>
> Summary
> -------
>
> Prevents buffer overflow bug by explicitly allowing RdmaIO layer to reserve header space in send buffers.
>
>
> This addresses bug QPID-3227.
> https://issues.apache.org/jira/browse/QPID-3227
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1096872
>
> Diff: https://reviews.apache.org/r/667/diff
>
>
> Testing
> -------
>
> unit tests & scale testing (by hand using perftest).
>
>
> Thanks,
>
> Kenneth
>
>
Re: Review Request: Reserve header bytes for RDMA send buffers.
Posted by Kenneth Giusti <kg...@apache.org>.
> On 2011-04-27 15:26:23, Andrew Stitcher wrote:
> > I recommend adding in this extra assertion to RdmaIO.cpp:
> > This would have caught the original bug.
> >
> > --- a/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
> > +++ b/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
> > @@ -213,6 +213,7 @@ namespace Rdma {
> > Buffer* ob = buff ? buff : getSendBuffer();
> > // Add FrameHeader after frame data
> > FrameHeader header(credit);
> > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount())
> > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize);
> > ob->dataCount(ob->dataCount()+FrameHeaderSize);
> > qp->postSend(ob);
> >
> >
>
> Andrew Stitcher wrote:
> Ken pointed out that this assertion would only be good for the old code and the new code needs to add in the reserved space too.
How about something like this (totally untested):
Index: cpp/src/qpid/sys/rdma/rdma_wrap.h
===================================================================
--- cpp/src/qpid/sys/rdma/rdma_wrap.h (revision 1097599)
+++ cpp/src/qpid/sys/rdma/rdma_wrap.h (working copy)
@@ -77,6 +77,8 @@
}
inline void Buffer::dataCount(int32_t s) {
+ // catch any attempt to overflow a buffer
+ assert(s <= bufferSize + reserved);
sge.length = s;
}
This stashes the check in the buffer code itself at the point where the application sets the length of the output data.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/667/#review577
-----------------------------------------------------------
On 2011-04-26 20:08:35, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/667/
> -----------------------------------------------------------
>
> (Updated 2011-04-26 20:08:35)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Chug Rolke.
>
>
> Summary
> -------
>
> Prevents buffer overflow bug by explicitly allowing RdmaIO layer to reserve header space in send buffers.
>
>
> This addresses bug QPID-3227.
> https://issues.apache.org/jira/browse/QPID-3227
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1096872
>
> Diff: https://reviews.apache.org/r/667/diff
>
>
> Testing
> -------
>
> unit tests & scale testing (by hand using perftest).
>
>
> Thanks,
>
> Kenneth
>
>
Re: Review Request: Reserve header bytes for RDMA send buffers.
Posted by Andrew Stitcher <as...@apache.org>.
> On 2011-04-27 15:26:23, Andrew Stitcher wrote:
> > I recommend adding in this extra assertion to RdmaIO.cpp:
> > This would have caught the original bug.
> >
> > --- a/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
> > +++ b/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
> > @@ -213,6 +213,7 @@ namespace Rdma {
> > Buffer* ob = buff ? buff : getSendBuffer();
> > // Add FrameHeader after frame data
> > FrameHeader header(credit);
> > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount())
> > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize);
> > ob->dataCount(ob->dataCount()+FrameHeaderSize);
> > qp->postSend(ob);
> >
> >
>
> Andrew Stitcher wrote:
> Ken pointed out that this assertion would only be good for the old code and the new code needs to add in the reserved space too.
>
> Kenneth Giusti wrote:
> How about something like this (totally untested):
>
> Index: cpp/src/qpid/sys/rdma/rdma_wrap.h
> ===================================================================
> --- cpp/src/qpid/sys/rdma/rdma_wrap.h (revision 1097599)
> +++ cpp/src/qpid/sys/rdma/rdma_wrap.h (working copy)
> @@ -77,6 +77,8 @@
> }
>
> inline void Buffer::dataCount(int32_t s) {
> + // catch any attempt to overflow a buffer
> + assert(s <= bufferSize + reserved);
> sge.length = s;
> }
>
>
> This stashes the check in the buffer code itself at the point where the application sets the length of the output data.
>
> Andrew Stitcher wrote:
> I think we could have both checks in there,
>
Especially as setting the size is actually done after you've already done the actual copying.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/667/#review577
-----------------------------------------------------------
On 2011-04-26 20:08:35, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/667/
> -----------------------------------------------------------
>
> (Updated 2011-04-26 20:08:35)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Chug Rolke.
>
>
> Summary
> -------
>
> Prevents buffer overflow bug by explicitly allowing RdmaIO layer to reserve header space in send buffers.
>
>
> This addresses bug QPID-3227.
> https://issues.apache.org/jira/browse/QPID-3227
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1096872
>
> Diff: https://reviews.apache.org/r/667/diff
>
>
> Testing
> -------
>
> unit tests & scale testing (by hand using perftest).
>
>
> Thanks,
>
> Kenneth
>
>
Re: Review Request: Reserve header bytes for RDMA send buffers.
Posted by Andrew Stitcher <as...@apache.org>.
> On 2011-04-27 15:26:23, Andrew Stitcher wrote:
> > I recommend adding in this extra assertion to RdmaIO.cpp:
> > This would have caught the original bug.
> >
> > --- a/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
> > +++ b/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
> > @@ -213,6 +213,7 @@ namespace Rdma {
> > Buffer* ob = buff ? buff : getSendBuffer();
> > // Add FrameHeader after frame data
> > FrameHeader header(credit);
> > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount())
> > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize);
> > ob->dataCount(ob->dataCount()+FrameHeaderSize);
> > qp->postSend(ob);
> >
> >
Ken pointed out that this assertion would only be good for the old code and the new code needs to add in the reserved space too.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/667/#review577
-----------------------------------------------------------
On 2011-04-26 20:08:35, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/667/
> -----------------------------------------------------------
>
> (Updated 2011-04-26 20:08:35)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Chug Rolke.
>
>
> Summary
> -------
>
> Prevents buffer overflow bug by explicitly allowing RdmaIO layer to reserve header space in send buffers.
>
>
> This addresses bug QPID-3227.
> https://issues.apache.org/jira/browse/QPID-3227
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1096872
>
> Diff: https://reviews.apache.org/r/667/diff
>
>
> Testing
> -------
>
> unit tests & scale testing (by hand using perftest).
>
>
> Thanks,
>
> Kenneth
>
>
Re: Review Request: Reserve header bytes for RDMA send buffers.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/667/#review577
-----------------------------------------------------------
I recommend adding in this extra assertion to RdmaIO.cpp:
This would have caught the original bug.
--- a/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
+++ b/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp
@@ -213,6 +213,7 @@ namespace Rdma {
Buffer* ob = buff ? buff : getSendBuffer();
// Add FrameHeader after frame data
FrameHeader header(credit);
+ assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount())
::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize);
ob->dataCount(ob->dataCount()+FrameHeaderSize);
qp->postSend(ob);
- Andrew
On 2011-04-26 20:08:35, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/667/
> -----------------------------------------------------------
>
> (Updated 2011-04-26 20:08:35)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Chug Rolke.
>
>
> Summary
> -------
>
> Prevents buffer overflow bug by explicitly allowing RdmaIO layer to reserve header space in send buffers.
>
>
> This addresses bug QPID-3227.
> https://issues.apache.org/jira/browse/QPID-3227
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1096872
>
> Diff: https://reviews.apache.org/r/667/diff
>
>
> Testing
> -------
>
> unit tests & scale testing (by hand using perftest).
>
>
> Thanks,
>
> Kenneth
>
>
Re: Review Request: Reserve header bytes for RDMA send buffers.
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/667/#review572
-----------------------------------------------------------
Ship it!
Looks right to me
- Gordon
On 2011-04-26 20:08:35, Kenneth Giusti wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/667/
> -----------------------------------------------------------
>
> (Updated 2011-04-26 20:08:35)
>
>
> Review request for qpid, Andrew Stitcher, Gordon Sim, and Chug Rolke.
>
>
> Summary
> -------
>
> Prevents buffer overflow bug by explicitly allowing RdmaIO layer to reserve header space in send buffers.
>
>
> This addresses bug QPID-3227.
> https://issues.apache.org/jira/browse/QPID-3227
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/qpid/sys/rdma/RdmaIO.cpp 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.h 1096872
> /trunk/qpid/cpp/src/qpid/sys/rdma/rdma_wrap.cpp 1096872
>
> Diff: https://reviews.apache.org/r/667/diff
>
>
> Testing
> -------
>
> unit tests & scale testing (by hand using perftest).
>
>
> Thanks,
>
> Kenneth
>
>