Thrift
  1. Thrift
  2. THRIFT-1414

bufferoverflow in c_glib buffered transport/socket client

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.7
    • Fix Version/s: None
    • Component/s: C glib - Library
    • Labels:
      None
    • Environment:

      Server running on Windows 7 SP1 64bit based on csharp.
      Client running on Ubuntu 11.04 Server 64 bit (fresh install) based on c_glib.
      svn rev: 1190015M

    • Patch Info:
      Patch Available

      Description

      Quote of comment in source:
      -----------------------------------------------
      if the buffer is still smaller than what we
      want to read, then just read it directly.
      -----------------------------------------------

      But the code reading into the tempdata with size of the buffer and reading all data into this.
      file: lib/c_glib/transport/thrift_buffered_transport.c line 74/98

      Also if the buffer is still bigger that what we want to read, then reading the buffer size.
      But recv blocks than and waiting of data if there nothing to read after the receiving data len.
      file: lib/c_glib/transport/thrift_buffered_transport.c line 118

      i attached a patch that fix this problems but i dont know if all of this is correct.

      1. thrift-0.9.0-c_glib-jira1414.patch
        1 kB
        Maik Greubel
      2. THRIFT-1414.patch
        2 kB
        Christian Zimnick

        Issue Links

          Activity

          Henrique Mendonça made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Hide
          Hudson added a comment -

          Integrated in Thrift #566 (See https://builds.apache.org/job/Thrift/566/)
          THRIFT-1414 bufferoverflow in c_glib buffered transport/socket client
          Patch: Christian Zimnick (Revision 1399452)

          Result = ABORTED
          roger : http://svn.apache.org/viewvc/?view=rev&rev=1399452
          Files :

          • /thrift/trunk/lib/c_glib/src/thrift/transport/thrift_buffered_transport.c
          • /thrift/trunk/lib/c_glib/src/thrift/transport/thrift_socket.c
          Show
          Hudson added a comment - Integrated in Thrift #566 (See https://builds.apache.org/job/Thrift/566/ ) THRIFT-1414 bufferoverflow in c_glib buffered transport/socket client Patch: Christian Zimnick (Revision 1399452) Result = ABORTED roger : http://svn.apache.org/viewvc/?view=rev&rev=1399452 Files : /thrift/trunk/lib/c_glib/src/thrift/transport/thrift_buffered_transport.c /thrift/trunk/lib/c_glib/src/thrift/transport/thrift_socket.c
          Roger Meier made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Roger Meier added a comment -

          thanks!

          Show
          Roger Meier added a comment - thanks!
          Maik Greubel made changes -
          Attachment thrift-0.9.0-c_glib-jira1414.patch [ 12549460 ]
          Hide
          Maik Greubel added a comment -

          The attached patch can be applied to 0.9.0.

          --- thrift-0.9.0/lib/c_glib/src/thrift/transport/thrift_socket.c.orig	2012-10-17 09:15:16.000000000 +0200
          +++ thrift-0.9.0/lib/c_glib/src/thrift/transport/thrift_socket.c	2012-10-17 09:15:48.000000000 +0200
          @@ -129,7 +129,7 @@
           
             while (got < len)
             {
          -    ret = recv (socket->sd, buf, len, 0);
          +    ret = recv (socket->sd, buf+got, len-got, 0);
               if (ret < 0)
               {
                 g_set_error (error, THRIFT_TRANSPORT_ERROR,
          --- thrift-0.9.0/lib/c_glib/src/thrift/transport/thrift_buffered_transport.c.orig	2012-10-17 09:16:07.000000000 +0200
          +++ thrift-0.9.0/lib/c_glib/src/thrift/transport/thrift_buffered_transport.c	2012-10-17 09:17:26.000000000 +0200
          @@ -71,7 +71,7 @@
             ThriftBufferedTransport *t = THRIFT_BUFFERED_TRANSPORT (transport);
             guint32 want = len;
             guint32 got = 0;
          -  guchar tmpdata[t->r_buf_size];
          +  guchar tmpdata[len];
             guint32 have = t->r_buf->len;
           
             // we shouldn't hit this unless the buffer doesn't have enough to read
          @@ -101,7 +101,7 @@
             } else {
               got += THRIFT_TRANSPORT_GET_CLASS (t->transport)->read (t->transport,
                                                                       tmpdata,
          -                                                            t->r_buf_size,
          +                                                            want,
                                                                       error);
               t->r_buf = g_byte_array_append (t->r_buf, tmpdata, got);
               
          
          
          Show
          Maik Greubel added a comment - The attached patch can be applied to 0.9.0. --- thrift-0.9.0/lib/c_glib/src/thrift/transport/thrift_socket.c.orig 2012-10-17 09:15:16.000000000 +0200 +++ thrift-0.9.0/lib/c_glib/src/thrift/transport/thrift_socket.c 2012-10-17 09:15:48.000000000 +0200 @@ -129,7 +129,7 @@ while (got < len) { - ret = recv (socket->sd, buf, len, 0); + ret = recv (socket->sd, buf+got, len-got, 0); if (ret < 0) { g_set_error (error, THRIFT_TRANSPORT_ERROR, --- thrift-0.9.0/lib/c_glib/src/thrift/transport/thrift_buffered_transport.c.orig 2012-10-17 09:16:07.000000000 +0200 +++ thrift-0.9.0/lib/c_glib/src/thrift/transport/thrift_buffered_transport.c 2012-10-17 09:17:26.000000000 +0200 @@ -71,7 +71,7 @@ ThriftBufferedTransport *t = THRIFT_BUFFERED_TRANSPORT (transport); guint32 want = len; guint32 got = 0; - guchar tmpdata[t->r_buf_size]; + guchar tmpdata[len]; guint32 have = t->r_buf->len; // we shouldn't hit this unless the buffer doesn't have enough to read @@ -101,7 +101,7 @@ } else { got += THRIFT_TRANSPORT_GET_CLASS (t->transport)->read (t->transport, tmpdata, - t->r_buf_size, + want, error); t->r_buf = g_byte_array_append (t->r_buf, tmpdata, got);
          Hide
          Andris Mednis added a comment -

          The patch was contributed by Christian Zimnick.

          Show
          Andris Mednis added a comment - The patch was contributed by Christian Zimnick.
          Jake Farrell made changes -
          Resolution Fixed [ 1 ]
          Status Closed [ 6 ] Reopened [ 4 ]
          Hide
          Jake Farrell added a comment -

          Andris, not sure why the status was changed not his ticket without the patch being added. Can you please rebase the patch against trunk. thanks

          Show
          Jake Farrell added a comment - Andris, not sure why the status was changed not his ticket without the patch being added. Can you please rebase the patch against trunk. thanks
          Hide
          Andris Mednis added a comment -

          Can you tell where is this fix ? Cannot find it in trunk yet.

          Show
          Andris Mednis added a comment - Can you tell where is this fix ? Cannot find it in trunk yet.
          Jake Farrell made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Roger Meier made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Roger Meier added a comment -

          duplicate was fixed: THRIFT-1654

          Show
          Roger Meier added a comment - duplicate was fixed: THRIFT-1654
          Jake Farrell made changes -
          Link This issue is duplicated by THRIFT-1654 [ THRIFT-1654 ]
          Christian Zimnick made changes -
          Field Original Value New Value
          Attachment THRIFT-1414.patch [ 12501520 ]
          Christian Zimnick created issue -

            People

            • Assignee:
              Unassigned
              Reporter:
              Christian Zimnick
            • Votes:
              3 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 10m
                10m
                Remaining:
                Remaining Estimate - 10m
                10m
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development