Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix possible memory leak in walletconsole/Util.cpp #921

Merged
merged 1 commit into from
Apr 14, 2020

Conversation

draane
Copy link
Contributor

@draane draane commented Apr 10, 2020

Description

Add missing call to delete in function fileR.

Testing instructions

The error was detected using cppcheck. For example you can execute cppcheck --quiet --force walletconsole/

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • If there is a related Issue, mention it in the description (e.g. 'Fixes Adopt Zilliqa to bech32 address format #444 ).

Add missing call to `delete` in function `fileR`.
@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #921 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #921      +/-   ##
==========================================
- Coverage   92.54%   92.53%   -0.01%     
==========================================
  Files         418      418              
  Lines       12161    12162       +1     
==========================================
  Hits        11254    11254              
- Misses        907      908       +1     
Impacted Files Coverage Δ
walletconsole/lib/Util.cpp 92.00% <0.00%> (-1.25%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d91994...b79f1d2. Read the comment docs.

Copy link
Contributor

@optout21 optout21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the improvement.

@optout21
Copy link
Contributor

Could you tell the exact way to find this finding? I was not able to reproduce it with cppcheck 1.90.

$ cppcheck --version
Cppcheck 1.90
$ cppcheck --force --quiet --enable=all walletconsole
walletconsole/lib/Coins.h:31:5: style: Class 'Coins' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
    Coins(ostream& out) : _out(out) {}
    ^
walletconsole/lib/Buffer.h:25:5: style: Class 'SavedValue' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
    SavedValue(const string& v) : _val(v) {}
    ^
walletconsole/lib/Buffer.h:37:5: style: Class 'Buffer' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
    Buffer(ostream& out) : _out(out) {}
    ^
walletconsole/lib/Util.h:21:5: style: Class 'Util' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
    Util(ostream& out) : _out(out) {}
    ^
walletconsole/lib/CommandExecutor.h:37:5: style: Class 'CommandExecutor' has a constructor with 1 argument that is not explicit. [noExplicitConstructor]
    CommandExecutor(ostream& out);
    ^
nofile:0:0: information: Cppcheck cannot find all the include files (use --check-config for details) [missingInclude]

(see issue #922 )

@optout21 optout21 merged commit 4fd327a into trustwallet:master Apr 14, 2020
@draane
Copy link
Contributor Author

draane commented Apr 21, 2020

@catenocrypt are you executing it on the non-fixed code? Because this is the output that I get:

$ cppcheck --version
Cppcheck 1.82

$ cppcheck --force --quiet  walletconsole/
[walletconsole/lib/Util.cpp:82]: (error) Memory leak: buffer

$ cppcheck --force --quiet --enable=all  walletconsole/
[walletconsole/lib/Coins.h:31]: (style) Class 'Coins' has a constructor with 1 argument that is not explicit.
[walletconsole/lib/Buffer.h:25]: (style) Class 'SavedValue' has a constructor with 1 argument that is not explicit.
[walletconsole/lib/Buffer.h:37]: (style) Class 'Buffer' has a constructor with 1 argument that is not explicit.
[walletconsole/lib/Util.h:21]: (style) Class 'Util' has a constructor with 1 argument that is not explicit.
[walletconsole/lib/CommandExecutor.h:37]: (style) Class 'CommandExecutor' has a constructor with 1 argument that is not explicit.
[walletconsole/lib/CommandExecutor.cpp:29]: (warning) Member variable 'CommandExecutor::_out' is not initialized in the constructor.
[walletconsole/lib/Keys.cpp:23]: (warning) Member variable 'Keys::_out' is not initialized in the constructor.
[walletconsole/lib/Keys.cpp:23]: (warning) Member variable 'Keys::_coins' is not initialized in the constructor.
[walletconsole/lib/Util.cpp:82]: (error) Memory leak: buffer
(information) Cppcheck cannot find all the include files (use --check-config for details)

My version of cppcheck is slightly older than yours but I don't think that it is the problem.

@draane
Copy link
Contributor Author

draane commented Apr 21, 2020

Looks like I was wrong in the previous comment. The problem seems to actually be in the newer version of cppcheck. I installed the latest version (1.90) compiling from source and it doesn't detect the memory leak any more.

@optout21
Copy link
Contributor

Thanks for checking. I just wanted to reproduce, to see if there are any other similar findings. Because the finding you reported was a genuine memory leak, albeit minor and low probability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adopt Zilliqa to bech32 address format
3 participants