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

Minor memory fix #550

Merged
merged 4 commits into from Jul 28, 2019

Conversation

@a-bezrukov
Copy link
Contributor

commented Jul 26, 2019

PR intention

Fixes a minor memory leak.

Andrey added some commits Jul 26, 2019

Andrey
Andrey
Andrey
Revert "Dealing with the race"
This reverts commit 1ab0a50.
@levonpetrosyan93
Copy link
Collaborator

left a comment

LGTM

@psolstice
Copy link
Contributor

left a comment

@reubenyap reubenyap added this to the v0.13.8.3 milestone Jul 26, 2019

@reubenyap reubenyap added this to In progress in Zcoin Core via automation Jul 26, 2019

@a-bezrukov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

Take a fix from the bitcoin code instead
https://github.com/bitcoin/bitcoin/blob/master/src/httprpc.cpp#L114

They have a static keyword for the char buffer. Our code does not have anything to prevent the race condition on the array. I understand why you requested this change but I don't know what is better for you in this situation - leave it as implemented (without unnecessary copying from char array to vector), or apply their code without the static keyword. Please let me know.

@psolstice

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

Look carefully at their code

        static const unsigned int KEY_SIZE = 32;
        unsigned char out[KEY_SIZE];

static is only for size (rightfully so though I'd prefer constexpr). Buffer is not static

Andrey

Zcoin Core automation moved this from In progress to Needs review Jul 27, 2019

@reubenyap reubenyap requested a review from psolstice Jul 27, 2019

Zcoin Core automation moved this from Needs review to Reviewer approved Jul 27, 2019

@reubenyap reubenyap merged commit 86d63df into master Jul 28, 2019

1 of 3 checks passed

LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

Zcoin Core automation moved this from Reviewer approved to Done Jul 28, 2019

@a-bezrukov a-bezrukov deleted the leak_fix branch Aug 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.