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

          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
          Hide
          Roger Meier added a comment -

          thanks!

          Show
          Roger Meier added a comment - thanks!
          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.
          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.
          Hide
          Roger Meier added a comment -

          duplicate was fixed: THRIFT-1654

          Show
          Roger Meier added a comment - duplicate was fixed: THRIFT-1654

            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