Qpid
  1. Qpid
  2. QPID-3227

rdma layer may allow overrun of send buffers

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.10
    • Fix Version/s: 0.11
    • Component/s: C++ Broker
    • Labels:
      None

      Description

      The rdma driver adds a small trailer to outbound buffers, however the size of this header is not accounted for when the buffer's size is passed to the codec. If the codec fills all available buffer space, the rdma driver will overwrite the end of the buffer when adding the trailer.

      Kudos to Chuck Rolke for helping root-cause this bug!

        Activity

        Justin Ross made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Ken Giusti made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. > Buffer* ob = buff ? buff : getSendBuffer(); bq. > // Add FrameHeader after frame data bq. > FrameHeader header(credit); bq. > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount()) bq. > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize); bq. > ob->dataCount(ob->dataCount()+FrameHeaderSize); bq. > qp->postSend(ob); bq. > bq. > bq. bq. Andrew Stitcher wrote: bq. 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. bq. bq. Kenneth Giusti wrote: bq. How about something like this (totally untested): bq. bq. Index: cpp/src/qpid/sys/rdma/rdma_wrap.h bq. =================================================================== bq. --- cpp/src/qpid/sys/rdma/rdma_wrap.h (revision 1097599) bq. +++ cpp/src/qpid/sys/rdma/rdma_wrap.h (working copy) bq. @@ -77,6 +77,8 @@ bq. }

        inline void Buffer::dataCount(int32_t s) { bq. + // catch any attempt to overflow a buffer bq. + assert(s <= bufferSize + reserved); bq. sge.length = s; bq. }

        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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. > Buffer* ob = buff ? buff : getSendBuffer(); bq. > // Add FrameHeader after frame data bq. > FrameHeader header(credit); bq. > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount()) bq. > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize); bq. > ob->dataCount(ob->dataCount()+FrameHeaderSize); bq. > qp->postSend(ob); bq. > bq. > bq. bq. Andrew Stitcher wrote: bq. 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. bq. bq. Kenneth Giusti wrote: bq. How about something like this (totally untested): bq. bq. Index: cpp/src/qpid/sys/rdma/rdma_wrap.h bq. =================================================================== bq. --- cpp/src/qpid/sys/rdma/rdma_wrap.h (revision 1097599) bq. +++ cpp/src/qpid/sys/rdma/rdma_wrap.h (working copy) bq. @@ -77,6 +77,8 @@ bq. } inline void Buffer::dataCount(int32_t s) { bq. + // catch any attempt to overflow a buffer bq. + assert(s <= bufferSize + reserved); bq. sge.length = s; bq. } 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. > Buffer* ob = buff ? buff : getSendBuffer(); bq. > // Add FrameHeader after frame data bq. > FrameHeader header(credit); bq. > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount()) bq. > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize); bq. > ob->dataCount(ob->dataCount()+FrameHeaderSize); bq. > qp->postSend(ob); bq. > bq. > bq. bq. Andrew Stitcher wrote: bq. 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. bq. bq. Kenneth Giusti wrote: bq. How about something like this (totally untested): bq. bq. Index: cpp/src/qpid/sys/rdma/rdma_wrap.h bq. =================================================================== bq. --- cpp/src/qpid/sys/rdma/rdma_wrap.h (revision 1097599) bq. +++ cpp/src/qpid/sys/rdma/rdma_wrap.h (working copy) bq. @@ -77,6 +77,8 @@ bq. }

        inline void Buffer::dataCount(int32_t s) { bq. + // catch any attempt to overflow a buffer bq. + assert(s <= bufferSize + reserved); bq. sge.length = s; bq. }

        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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. > Buffer* ob = buff ? buff : getSendBuffer(); bq. > // Add FrameHeader after frame data bq. > FrameHeader header(credit); bq. > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount()) bq. > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize); bq. > ob->dataCount(ob->dataCount()+FrameHeaderSize); bq. > qp->postSend(ob); bq. > bq. > bq. bq. Andrew Stitcher wrote: bq. 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. bq. bq. Kenneth Giusti wrote: bq. How about something like this (totally untested): bq. bq. Index: cpp/src/qpid/sys/rdma/rdma_wrap.h bq. =================================================================== bq. --- cpp/src/qpid/sys/rdma/rdma_wrap.h (revision 1097599) bq. +++ cpp/src/qpid/sys/rdma/rdma_wrap.h (working copy) bq. @@ -77,6 +77,8 @@ bq. } inline void Buffer::dataCount(int32_t s) { bq. + // catch any attempt to overflow a buffer bq. + assert(s <= bufferSize + reserved); bq. sge.length = s; bq. } 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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 { bq. > Buffer* ob = buff ? buff : getSendBuffer(); bq. > // Add FrameHeader after frame data bq. > FrameHeader header(credit); bq. > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount()) bq. > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize); bq. > ob->dataCount(ob->dataCount()+FrameHeaderSize); bq. > qp->postSend(ob); bq. > bq. > bq. bq. Andrew Stitcher wrote: bq. 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

        Show
        jiraposter@reviews.apache.org added a comment - 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 { bq. > Buffer* ob = buff ? buff : getSendBuffer(); bq. > // Add FrameHeader after frame data bq. > FrameHeader header(credit); bq. > + assert(int32_t(ob->dataCount()+FrameHeaderSize) <= ob->byteCount()) bq. > ::memcpy(ob->bytes()+ob->dataCount(), &header, FrameHeaderSize); bq. > ob->dataCount(ob->dataCount()+FrameHeaderSize); bq. > qp->postSend(ob); bq. > bq. > bq. bq. Andrew Stitcher wrote: bq. 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
        Hide
        jiraposter@reviews.apache.org added a comment -

        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

        Show
        jiraposter@reviews.apache.org added a comment - 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
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Hide
        jiraposter@reviews.apache.org added a comment -

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

        Show
        jiraposter@reviews.apache.org added a comment - ----------------------------------------------------------- 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
        Ken Giusti made changes -
        Field Original Value New Value
        Attachment QPID-3227.patch [ 12477437 ]
        Hide
        Ken Giusti added a comment -

        proposed patch.

        Show
        Ken Giusti added a comment - proposed patch.
        Ken Giusti created issue -

          People

          • Assignee:
            Ken Giusti
            Reporter:
            Ken Giusti
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development