Thrift
  1. Thrift
  2. THRIFT-1522

TServerSocket potential memory leak with addrinfo *res0

    Details

      Description

      I noticed a potential memory leak in TServerSocket.cpp. The res0 pointer receives an allocation from getaddrinfo() but is only freed part of the time in a conditional branch. It should be outside of the if-else to ensure it's deallocated every time.

      I'm holding off on creating a patch in case I'm not understanding some hidden functionality in the socket library. Here is the code snippet:

      struct addrinfo *res0;
      ...
      getaddrinfo(NULL, port, &hints, &res0);
      ...
      if (! path_.empty())

      { ... }

      else

      { ... freeaddrinfo(res0); //Line 331 of TServerSocket.cpp }

        Issue Links

          Activity

          Hide
          Claudius Heine added a comment - - edited

          I would say your assessment of the memory leak is correct.
          It is never deallocated when an exception is thrown or it has to open a Unix Domain Socket.
          The memory leak is present in the current commit in the master branch (5cf9d7744c41).
          IMO the best solution would be, to wrap getaddrinfo and freeaddrinfo into a class that calls the methods with its con/destructors.

          Show
          Claudius Heine added a comment - - edited I would say your assessment of the memory leak is correct. It is never deallocated when an exception is thrown or it has to open a Unix Domain Socket. The memory leak is present in the current commit in the master branch (5cf9d7744c41). IMO the best solution would be, to wrap getaddrinfo and freeaddrinfo into a class that calls the methods with its con/destructors.
          Hide
          Roger Meier added a comment -

          Claudius Heine could you please provide a patch for this?

          Show
          Roger Meier added a comment - Claudius Heine could you please provide a patch for this?
          Hide
          Claudius Heine added a comment -

          Ok, I will take a look.

          Show
          Claudius Heine added a comment - Ok, I will take a look.
          Hide
          ASF GitHub Bot added a comment -

          GitHub user cmhe opened a pull request:

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

          THRIFT-1522: Fixes Memory leak by wrapping getaddrinfo into a class.

          Sponsored-by: Roger Meier <r.meier@siemens.com>
          Signed-off-by: Claudius Heine <ch@denx.de>

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

          $ git pull https://github.com/cmhe/thrift THRIFT-1522

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

          https://github.com/apache/thrift/pull/542.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 #542


          commit 4828e52ffe6a8bd0cdd7c40b629143a5ec299e31
          Author: Claudius Heine <ch@denx.de>
          Date: 2015-07-06T07:14:25Z

          THRIFT-1522: Fixes Memory leak by wrapping getaddrinfo into a class.

          Sponsored-by: Roger Meier <r.meier@siemens.com>
          Signed-off-by: Claudius Heine <ch@denx.de>


          Show
          ASF GitHub Bot added a comment - GitHub user cmhe opened a pull request: https://github.com/apache/thrift/pull/542 THRIFT-1522 : Fixes Memory leak by wrapping getaddrinfo into a class. Sponsored-by: Roger Meier <r.meier@siemens.com> Signed-off-by: Claudius Heine <ch@denx.de> You can merge this pull request into a Git repository by running: $ git pull https://github.com/cmhe/thrift THRIFT-1522 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/542.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 #542 commit 4828e52ffe6a8bd0cdd7c40b629143a5ec299e31 Author: Claudius Heine <ch@denx.de> Date: 2015-07-06T07:14:25Z THRIFT-1522 : Fixes Memory leak by wrapping getaddrinfo into a class. Sponsored-by: Roger Meier <r.meier@siemens.com> Signed-off-by: Claudius Heine <ch@denx.de>
          Hide
          ASF GitHub Bot added a comment -

          Github user cmhe commented on the pull request:

          https://github.com/apache/thrift/pull/542#issuecomment-118762338

          I have run "make cross" and "make check" without any unexpected errors.
          Feedback is welcome.

          Show
          ASF GitHub Bot added a comment - Github user cmhe commented on the pull request: https://github.com/apache/thrift/pull/542#issuecomment-118762338 I have run "make cross" and "make check" without any unexpected errors. Feedback is welcome.
          Hide
          ASF GitHub Bot added a comment -

          Github user cmhe commented on the pull request:

          https://github.com/apache/thrift/pull/542#issuecomment-118833111

          Maybe this can also be used in TNonblockingServer.cpp and TSocket.cpp.

          Show
          ASF GitHub Bot added a comment - Github user cmhe commented on the pull request: https://github.com/apache/thrift/pull/542#issuecomment-118833111 Maybe this can also be used in TNonblockingServer.cpp and TSocket.cpp.
          Hide
          ASF GitHub Bot added a comment -

          Github user cmhe commented on the pull request:

          https://github.com/apache/thrift/pull/542#issuecomment-118848681

          I moved TGetAddrInfoWrapper into its own C++ Module and integrated it into TNonblockingServer and TSocket: https://github.com/cmhe/thrift/commits/WrappedGetAddrInfo
          Comments are welcome.

          Show
          ASF GitHub Bot added a comment - Github user cmhe commented on the pull request: https://github.com/apache/thrift/pull/542#issuecomment-118848681 I moved TGetAddrInfoWrapper into its own C++ Module and integrated it into TNonblockingServer and TSocket: https://github.com/cmhe/thrift/commits/WrappedGetAddrInfo Comments are welcome.

            People

            • Assignee:
              Unassigned
              Reporter:
              Peace C
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:

                Development