-
Notifications
You must be signed in to change notification settings - Fork 21
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
Memory retention discussion - not a bug (yet?) #28
Comments
An example valgrind finding from running xcashd for around 10 minutes.
And another, this time from within tools::log_stack_trace() :
|
Hi @CygnusMiner If you can continue testing and find a code fix for the slow mem leak, we can do a bug bounty on this for you. Its because this is an important problem, and yes its a slow mem leak as it takes weeks for it to stall |
Ok, I've been squirreled by a separate finding from valgrind (below) - an actual memory leak this time :) - 2MB per allocate. Tracing it down, it seems to be occurring here:
The commenter seems to think that the hp_state thread pointer should already have been allocated, but the only deliberate allocation I see for the pointer is in miner.cpp and in blockchain.cpp, in both cases they specifically free the allocation after they're done with it (miner.cpp and blockchain.cpp). This allocation seems to occur per thread so it should be a limited memory leak (unless threads are destroyed and recreated often in here - I don't know the Monero/Xcash code that well). However, limited or not it looks like the best approach would be to remove the conditional:
explicitly allocating it, and then call the slow_hash_free_state() after you're done with hp_state pointer, somewhere around here Also, as an aside and just to apease the Paranoid OCD in me, wouldn't it be prudent to memset the block prior to freeing after each use in encrypting? Just to reduce the chance of any accidental loss of sensitive data from a forced core dump? For reference, one of the findings from Valgrind:
|
Also Valgrind is upset that you're testing an uninitialized array in get_network_block_database_hash() |
Thanks for more work on this! I can go ahead and add the new find to the testnet and we can test it out in a few days. I will look more into that area as I am not so familiar with that particular section of that file. For the memset I would think so, thats how dpops was built because of that risk, so I will need to look into that as well. For the "xcash original" code part yes that makes sense looking at it now, so that would be good to fix as well. Let keep this going with your findings on this page and I can reference them once we add them to the testnet |
There's also an issue on exit which I experienced during memory testing. It seems that a mutex variable is getting destroyed at daemon shutdown, followed by an attempt to unlock using that mutex. The result's a thrown boost::lock_error exception from mlocker::unlock() call. We're exiting the daemon at this point, so it's not a big deal, but it's causing an abort prior to clean-up which is messing around with the memory checkers. I hacked my way around it by throwing away the exception with a try/catch wrapper (below) but that's hardly a valid fix.
Mostly just making note of it here as an item to track down how to properly address the use of the mlocker after it's been destroyed. Valgrind output follows:
Oh, and also there's another uninitialized variable in wallet2.cpp height
|
So, looking through valgrind and dr_memory reports, I'm seeing no explicit memory leaks (all allocated memory is still reachable). However, I am seeing what looks like a memory retention/bloat issue with easylogging++. It seems that every time the logger is used to write, it instantiates a new el::Logger object, which calls el::Logger::configure(), eventually resulting in an el::Configurations::unsafeSet() allocation that gets pushed onto a list.
Culprit seems to be here:
which does the push:
I'm not sure if/when it ever gets popped back off of this stack. And this seems to happen repetitively. My guess is that there is some sort of cleanup that needs to be happening in the xcash/monero core code after each write (or somehow convincing easylogging++ not to instantiate a Logger object every single time).
For example, this MTRACE() macro from cryptonote_protocol_handler.inl seems to be one of the instigators:
Still researching it, though...
The text was updated successfully, but these errors were encountered: