qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Stitcher" <astitc...@apache.org>
Subject Re: Review Request: Reserve header bytes for RDMA send buffers.
Date Thu, 28 Apr 2011 21:24:41 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message