Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-4331

C++: TSSLSockets bug in handling huge messages, bug in handling polling

Details

    • Bug
    • Status: Closed
    • Major
    • Resolution: Fixed
    • 0.10.0
    • 0.11.0
    • C++ - Library
    • None
    • Patch Available
    • Patch, Important

    Description

      The TSSLSocket class did not handle large messages, because a underlying TCP socket may signal bytes received, while SSL_read() may not have bytes available. After maxretries (5) the function returned -1, which got interpreted as unsigned integer for read bytes.

      Futher the waitForEvent methode, did only set THRIFT_POLLIN or THRIFT_POLLOUT, but it gets used where SSL needs to send AND receive bytes for some operations (like close). So in the case of write wanted, THRIFT_POLLIN is also set to cover these read/write operations.

      Pullrequest for master and 0.10.0 branch will follow.

      Attachments

        1. 0.10.0-THRIFT-4331.patch
          5 kB
          Martin Haimberger
        2. master-THRIFT-4331.patch
          5 kB
          Martin Haimberger

        Issue Links

          Activity

            patches for master and 0.10.0

            martinhaimberger Martin Haimberger added a comment - patches for master and 0.10.0
            githubbot ASF GitHub Bot added a comment -

            GitHub user MartinHaimberger opened a pull request:

            https://github.com/apache/thrift/pull/1363

            THRIFT-4331 - C++: TSSLSocket fixes

            fixed issue with large messages, where waitForEvent was called
            mutliple times waiting for SSL_read() to get bytes and running
            in the retry timeout.

            fixed issue where poll was not using the right flags.

            Fixes issue THRIFT-4331 on master.

            You can merge this pull request into a Git repository by running:

            $ git pull https://github.com/MartinHaimberger/thrift master

            Alternatively you can review and apply these changes as the patch at:

            https://github.com/apache/thrift/pull/1363.patch

            To close this pull request, make a commit to your master/trunk branch
            with (at least) the following in the commit message:

            This closes #1363


            commit 3e6e271dedbbb46f1a45f5e5d0b56bddf1589764
            Author: Martin Haimberger <martin.haimberger@thincast.com>
            Date: 2017-09-15T11:03:27Z

            C++: TSSLSocket fixes

            fixed issue with large messages, where waitForEvent was called
            mutliple times waiting for SSL_read() to get bytes and running
            in the retry timeout.

            fixed issue where poll was not using the right flags.


            githubbot ASF GitHub Bot added a comment - GitHub user MartinHaimberger opened a pull request: https://github.com/apache/thrift/pull/1363 THRIFT-4331 - C++: TSSLSocket fixes fixed issue with large messages, where waitForEvent was called mutliple times waiting for SSL_read() to get bytes and running in the retry timeout. fixed issue where poll was not using the right flags. Fixes issue THRIFT-4331 on master. You can merge this pull request into a Git repository by running: $ git pull https://github.com/MartinHaimberger/thrift master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/1363.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #1363 commit 3e6e271dedbbb46f1a45f5e5d0b56bddf1589764 Author: Martin Haimberger <martin.haimberger@thincast.com> Date: 2017-09-15T11:03:27Z C++: TSSLSocket fixes fixed issue with large messages, where waitForEvent was called mutliple times waiting for SSL_read() to get bytes and running in the retry timeout. fixed issue where poll was not using the right flags.
            githubbot ASF GitHub Bot added a comment -

            Github user jeking3 commented on a diff in the pull request:

            https://github.com/apache/thrift/pull/1363#discussion_r139214105

            — Diff: lib/cpp/src/thrift/transport/TSSLSocket.cpp —
            @@ -759,7 +775,9 @@ unsigned int TSSLSocket::waitForEvent(bool wantRead) {
            struct THRIFT_POLLFD fds[2];
            memset(fds, 0, sizeof(fds));
            fds[0].fd = fdSocket;

            • fds[0].events = wantRead ? THRIFT_POLLIN : THRIFT_POLLOUT;
              + // use POLLIN also on write operations too, this is needed for operations
              + // which requires read and write on the socket.
                • End diff –

            Interesting, what's an example of this?

            githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on a diff in the pull request: https://github.com/apache/thrift/pull/1363#discussion_r139214105 — Diff: lib/cpp/src/thrift/transport/TSSLSocket.cpp — @@ -759,7 +775,9 @@ unsigned int TSSLSocket::waitForEvent(bool wantRead) { struct THRIFT_POLLFD fds [2] ; memset(fds, 0, sizeof(fds)); fds [0] .fd = fdSocket; fds [0] .events = wantRead ? THRIFT_POLLIN : THRIFT_POLLOUT; + // use POLLIN also on write operations too, this is needed for operations + // which requires read and write on the socket. End diff – Interesting, what's an example of this?
            githubbot ASF GitHub Bot added a comment -

            Github user jeking3 commented on the issue:

            https://github.com/apache/thrift/pull/1362

            We don't retroactively fix branches; the community might be interested in this but as a pull request it won't be merged. @jfarrell this can be closed.

            githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1362 We don't retroactively fix branches; the community might be interested in this but as a pull request it won't be merged. @jfarrell this can be closed.
            githubbot ASF GitHub Bot added a comment -

            Github user MartinHaimberger commented on a diff in the pull request:

            https://github.com/apache/thrift/pull/1363#discussion_r139311583

            — Diff: lib/cpp/src/thrift/transport/TSSLSocket.cpp —
            @@ -759,7 +775,9 @@ unsigned int TSSLSocket::waitForEvent(bool wantRead) {
            struct THRIFT_POLLFD fds[2];
            memset(fds, 0, sizeof(fds));
            fds[0].fd = fdSocket;

            • fds[0].events = wantRead ? THRIFT_POLLIN : THRIFT_POLLOUT;
              + // use POLLIN also on write operations too, this is needed for operations
              + // which requires read and write on the socket.
                • End diff –

            hi jeking3,

            the comment may not be perfect, but SSL operations, like SSL_connect or SSL_shutdown (where this function is also used) need to perform write and read operations on the socket and we dont know where we are so we need to listen on both socket events.

            I could also introduce a new function parameter for that.

            githubbot ASF GitHub Bot added a comment - Github user MartinHaimberger commented on a diff in the pull request: https://github.com/apache/thrift/pull/1363#discussion_r139311583 — Diff: lib/cpp/src/thrift/transport/TSSLSocket.cpp — @@ -759,7 +775,9 @@ unsigned int TSSLSocket::waitForEvent(bool wantRead) { struct THRIFT_POLLFD fds [2] ; memset(fds, 0, sizeof(fds)); fds [0] .fd = fdSocket; fds [0] .events = wantRead ? THRIFT_POLLIN : THRIFT_POLLOUT; + // use POLLIN also on write operations too, this is needed for operations + // which requires read and write on the socket. End diff – hi jeking3, the comment may not be perfect, but SSL operations, like SSL_connect or SSL_shutdown (where this function is also used) need to perform write and read operations on the socket and we dont know where we are so we need to listen on both socket events. I could also introduce a new function parameter for that.
            githubbot ASF GitHub Bot added a comment -

            Github user MartinHaimberger closed the pull request at:

            https://github.com/apache/thrift/pull/1362

            githubbot ASF GitHub Bot added a comment - Github user MartinHaimberger closed the pull request at: https://github.com/apache/thrift/pull/1362
            githubbot ASF GitHub Bot added a comment -

            Github user jeking3 commented on the issue:

            https://github.com/apache/thrift/pull/1363

            If you could do me a favor and rebase on the current master, I finally got all the CI build issues resolved so I'd like to see a clean build before I merge. There are some minor formatting (indentation) issues as well.

            githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1363 If you could do me a favor and rebase on the current master, I finally got all the CI build issues resolved so I'd like to see a clean build before I merge. There are some minor formatting (indentation) issues as well.

            If you could do me a favor and rebase on the current master, I finally got all the CI build issues resolved so I'd like to see a clean build before I merge. There are some minor formatting (indentation) issues as well.

            jking3 James E. King III added a comment - If you could do me a favor and rebase on the current master, I finally got all the CI build issues resolved so I'd like to see a clean build before I merge. There are some minor formatting (indentation) issues as well.
            githubbot ASF GitHub Bot added a comment -

            Github user jeking3 commented on the issue:

            https://github.com/apache/thrift/pull/1363

            @MartinHaimberger If you could do me a favor and rebase on the current master, I finally got all the CI build issues resolved so I'd like to see a clean build before I merge. There are some minor formatting (indentation) issues as well.

            githubbot ASF GitHub Bot added a comment - Github user jeking3 commented on the issue: https://github.com/apache/thrift/pull/1363 @MartinHaimberger If you could do me a favor and rebase on the current master, I finally got all the CI build issues resolved so I'd like to see a clean build before I merge. There are some minor formatting (indentation) issues as well.
            githubbot ASF GitHub Bot added a comment -

            Github user MartinHaimberger commented on the issue:

            https://github.com/apache/thrift/pull/1363

            rebased and indentations should be fixed

            githubbot ASF GitHub Bot added a comment - Github user MartinHaimberger commented on the issue: https://github.com/apache/thrift/pull/1363 rebased and indentations should be fixed

            Committed - thanks.

            jking3 James E. King III added a comment - Committed - thanks.
            githubbot ASF GitHub Bot added a comment -

            Github user asfgit closed the pull request at:

            https://github.com/apache/thrift/pull/1363

            githubbot ASF GitHub Bot added a comment - Github user asfgit closed the pull request at: https://github.com/apache/thrift/pull/1363

            People

              jking3 James E. King III
              martinhaimberger Martin Haimberger
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: