You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/10/29 23:27:14 UTC

[GitHub] [geode-native] pdxcodemonkey opened a new pull request #682: GEMNC-488: Stop leaking buffer in CLI DataInput 

pdxcodemonkey opened a new pull request #682:
URL: https://github.com/apache/geode-native/pull/682


   The CLI DataInput object has two ctors, one of which copies the passed-in buffer parameter via new[] and one of which doesn't.  In the event that the former is called, the buffer is leaked when the object is deleted/Disposed.
   
   @mreddington @dihardman @karensmolermiller @davebarnes97 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #682:
URL: https://github.com/apache/geode-native/pull/682#discussion_r517493689



##########
File path: clicache/src/DataInput.cpp
##########
@@ -878,8 +877,6 @@ namespace Apache
 
       void DataInput::Cleanup()
       {

Review comment:
       Done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] codecov-io commented on pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #682:
URL: https://github.com/apache/geode-native/pull/682#issuecomment-719112394


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=h1) Report
   > Merging [#682](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=desc) into [develop](https://codecov.io/gh/apache/geode-native/commit/4bcb3db1e476dd0a73e9bffac6ea65ae7d55105a?el=desc) will **increase** coverage by `0.02%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/682/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #682      +/-   ##
   ===========================================
   + Coverage    73.99%   74.01%   +0.02%     
   ===========================================
     Files          644      644              
     Lines        51189    51189              
   ===========================================
   + Hits         37877    37890      +13     
   + Misses       13312    13299      -13     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [cppcache/src/PdxTypeRegistry.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1BkeFR5cGVSZWdpc3RyeS5jcHA=) | `77.94% <0.00%> (-2.21%)` | :arrow_down: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `75.94% <0.00%> (-0.08%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.24% <0.00%> (ø)` | |
   | [cppcache/src/TcrMessage.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1Rjck1lc3NhZ2UuY3Bw) | `85.72% <0.00%> (+0.10%)` | :arrow_up: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `54.69% <0.00%> (+0.42%)` | :arrow_up: |
   | [cppcache/src/ThinClientPoolDM.hpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uaHBw) | `90.32% <0.00%> (+1.61%)` | :arrow_up: |
   | [cppcache/src/ThinClientStickyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRTdGlja3lNYW5hZ2VyLmNwcA==) | `88.46% <0.00%> (+10.57%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=footer). Last update [4bcb3db...f587380](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] lgtm-com[bot] commented on pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #682:
URL: https://github.com/apache/geode-native/pull/682#issuecomment-721966498


   This pull request **introduces 4 alerts** when merging 0e9463d69e7be5455a351eae23b87cef9b2382ac into 9279098352e5c6440cade1196b9b99dcf89e90c5 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-a03165f373b94fecca20198e049a9105fc55bcb8)
   
   **new alerts:**
   
   * 2 for Call to GC\.Collect\(\)
   * 2 for Useless assignment to local variable


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #682:
URL: https://github.com/apache/geode-native/pull/682#discussion_r516176141



##########
File path: clicache/src/DataInput.cpp
##########
@@ -878,8 +877,6 @@ namespace Apache
 
       void DataInput::Cleanup()
       {

Review comment:
       If this method is empty perhaps we can remove it?

##########
File path: clicache/src/DataInput.hpp
##########
@@ -663,7 +664,7 @@ namespace Apache
           m_buffer = const_cast<System::Byte*>(nativeptr->currentBufferPosition());
           if ( m_buffer != NULL) {
             m_bufferLength = static_cast<decltype(m_bufferLength)>(nativeptr->getBytesRemaining());
-					}
+          }

Review comment:
       Some strange formatting mess here.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #682:
URL: https://github.com/apache/geode-native/pull/682#discussion_r517595960



##########
File path: clicache/src/DataInput.cpp
##########
@@ -93,8 +93,9 @@ namespace Apache
         if (buffer != nullptr && buffer->Length > 0) {
           _GF_MG_EXCEPTION_TRY2
 
-            System::Int32 len = buffer->Length;
-          _GEODE_NEW(m_buffer, System::Byte[len]);
+          System::Int32 len = buffer->Length;

Review comment:
       auto?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] lgtm-com[bot] commented on pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #682:
URL: https://github.com/apache/geode-native/pull/682#issuecomment-721943662


   This pull request **introduces 4 alerts** when merging 69f5a49c1d86d44445cb52cb6fe6ccbca5e27c87 into 9279098352e5c6440cade1196b9b99dcf89e90c5 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-9196608f3c466b1421ff234d7e093fcfb418615c)
   
   **new alerts:**
   
   * 2 for Call to GC\.Collect\(\)
   * 2 for Useless assignment to local variable


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #682:
URL: https://github.com/apache/geode-native/pull/682#discussion_r517490303



##########
File path: clicache/src/DataInput.hpp
##########
@@ -663,7 +664,7 @@ namespace Apache
           m_buffer = const_cast<System::Byte*>(nativeptr->currentBufferPosition());
           if ( m_buffer != NULL) {
             m_bufferLength = static_cast<decltype(m_bufferLength)>(nativeptr->getBytesRemaining());
-					}
+          }

Review comment:
       Appears to be okay now.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey merged pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #682:
URL: https://github.com/apache/geode-native/pull/682


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] codecov-io edited a comment on pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #682:
URL: https://github.com/apache/geode-native/pull/682#issuecomment-719112394


   # [Codecov](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=h1) Report
   > Merging [#682](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=desc) into [develop](https://codecov.io/gh/apache/geode-native/commit/0d9a99d5e0632de62df17921950cf3f6640efb33?el=desc) will **decrease** coverage by `0.01%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/geode-native/pull/682/graphs/tree.svg?width=650&height=150&src=pr&token=plpAqoqGag)](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=tree)
   
   ```diff
   @@             Coverage Diff             @@
   ##           develop     #682      +/-   ##
   ===========================================
   - Coverage    74.04%   74.02%   -0.02%     
   ===========================================
     Files          644      644              
     Lines        51189    51189              
   ===========================================
   - Hits         37903    37894       -9     
   - Misses       13286    13295       +9     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...test/testThinClientPoolExecuteHAFunctionPrSHOP.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvaW50ZWdyYXRpb24tdGVzdC90ZXN0VGhpbkNsaWVudFBvb2xFeGVjdXRlSEFGdW5jdGlvblByU0hPUC5jcHA=) | `91.20% <0.00%> (-3.71%)` | :arrow_down: |
   | [cppcache/src/ThinClientRedundancyManager.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWR1bmRhbmN5TWFuYWdlci5jcHA=) | `75.78% <0.00%> (-0.63%)` | :arrow_down: |
   | [cppcache/src/ClientMetadataService.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0NsaWVudE1ldGFkYXRhU2VydmljZS5jcHA=) | `62.24% <0.00%> (-0.46%)` | :arrow_down: |
   | [cppcache/src/ExecutionImpl.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL0V4ZWN1dGlvbkltcGwuY3Bw) | `68.07% <0.00%> (-0.39%)` | :arrow_down: |
   | [cppcache/src/ThinClientPoolDM.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRQb29sRE0uY3Bw) | `76.23% <0.00%> (-0.15%)` | :arrow_down: |
   | [cppcache/src/ThinClientRegion.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RoaW5DbGllbnRSZWdpb24uY3Bw) | `56.04% <0.00%> (-0.06%)` | :arrow_down: |
   | [cppcache/src/TcrEndpoint.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckVuZHBvaW50LmNwcA==) | `55.11% <0.00%> (+0.56%)` | :arrow_up: |
   | [cppcache/src/TcrConnection.cpp](https://codecov.io/gh/apache/geode-native/pull/682/diff?src=pr&el=tree#diff-Y3BwY2FjaGUvc3JjL1RjckNvbm5lY3Rpb24uY3Bw) | `73.27% <0.00%> (+0.78%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=footer). Last update [0d9a99d...69f5a49](https://codecov.io/gh/apache/geode-native/pull/682?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] lgtm-com[bot] commented on pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #682:
URL: https://github.com/apache/geode-native/pull/682#issuecomment-721919329


   This pull request **introduces 4 alerts** when merging 0588ef5947fe3875c7f2cb90732179ecbada8bfb into 9279098352e5c6440cade1196b9b99dcf89e90c5 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-ed3924809465bbcd002b9a6d79ae4ffd394a4423)
   
   **new alerts:**
   
   * 2 for Call to GC\.Collect\(\)
   * 2 for Useless assignment to local variable


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] lgtm-com[bot] commented on pull request #682: GEODE-8647: Stop leaking buffer in CLI DataInput

Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #682:
URL: https://github.com/apache/geode-native/pull/682#issuecomment-721882183


   This pull request **introduces 4 alerts** when merging daac2c91545b9c8cb10d729e741658eb463deac2 into 0d9a99d5e0632de62df17921950cf3f6640efb33 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode-native/rev/pr-60291afa9c24d4b295d3b0886ebe8f339141f43c)
   
   **new alerts:**
   
   * 2 for Call to GC\.Collect\(\)
   * 2 for Useless assignment to local variable


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org