Fix memory leak in instantiation_cache #2701

learnforpractice · 2018-05-02T07:42:11Z

No description provided.

learnforpractice · 2018-05-02T07:43:19Z

learnforpractice · 2018-05-02T08:02:15Z

In order to break the module cache check, eosio.token need a small hack, place the following code in eosio.token.cpp, testing code will replace "hello,world" with other characters in order to workaround the module cache check.

void sayHello() {
   prints("hello,world");
}

Run the following testing code under build/contracts

import os
import shlex
import subprocess

def toname(i):
    return chr(ord('A')+int(i%10)) + chr(ord('A')+int((i/10)%10)) + chr(ord('A')+int((i/100)%10))

cleos = './../programs/cleos/cleos'

create_account_cmd = '{0} create account eosio eosio.token EOS7ijWCBmoXBi3CgtK7DJxentZZeTkeUnaSDvyro9dq7Sd1C3dC4 EOS7ijWCBmoXBi3CgtK7DJxentZZeTkeUnaSDvyro9dq7Sd1C3dC4'.format(cleos)
create_account_cmd = shlex.split(create_account_cmd)
ret = subprocess.call(create_account_cmd)
'''
if ret != 0:
    raise Exception('Creating account failed')
'''

wast = 'eosio.token/eosio.token.wast'
if not os.path.exists(wast+'2'):
    with open(wast, 'rb') as f:
        with open(wast+'2', 'wb') as f2:
            data = f.read()
            f2.write(data)

for i in range(1, 1000):
    key_words = b"hello,world"
    with open(wast+'2', 'rb') as f:
        data = f.read()
        #data.find(key_words)
        replace_str = b"a%d"%(i,)
        replace_str.zfill(len(key_words))
        #replace key works with custom words to break the effect of code cache mechanism
        print(data.find(key_words))
        data = data.replace(key_words, replace_str)
        with open(wast, 'wb') as f:
            f.write(data)

    ret = subprocess.call(['./../programs/cleos/cleos', 'set', 'contract', 'eosio.token', 'eosio.token',  '-p', 'eosio.token'])
    if ret != 0:
        raise Exception('Setting contract failed')

    ret = subprocess.call(['./../programs/cleos/cleos', 'push', 'action', 'eosio.token', 'create','[ "eosio", "%d.0000 %s", 0, 0, 0]'%(i,toname(i)), '-p', 'eosio.token'])

Expecting to see no dramatically memory increase during the running of testing code.

spoonincode · 2018-05-03T13:39:14Z

Keeping all the versions of code was an explicit choice at the time. Calling @wanderingbort what do you think?

wanderingbort · 2018-05-03T20:55:13Z

libraries/chain/include/eosio/chain/wasm_interface_private.hpp

@@ -19,6 +19,12 @@ using namespace Runtime;

 namespace eosio { namespace chain {

+   struct account_module {
+      digest_type digest;
+      std::shared_ptr<wasm_instantiated_module_interface> module_interface;


This doesn't seem to be used anymore, meaning the map<account_name, account_module> account_module_map can probably become a map<account_name, digest_type>

That would remove the need to change the std::unique_ptr to a std::shared_ptr as well which is nicer.

wanderingbort · 2018-05-03T20:59:23Z

tests/tests/misc_tests.cpp

+
+   auto wasm = ::eosio::chain::wast_to_wasm(code);
+
+   auto wasm_code = shared_vector<char>(alloc);


shared_vector is just a vector that has a different allocator. For the purposes of your test it could just be a vector<char> which would make the inclusion of bip::managed_mapped_file seg(bip::open_or_create,"./test.db", 1024*1024); and all the related code to derive alloc unnecessary

spoonincode · 2018-05-03T21:11:32Z

tests/tests/misc_tests.cpp

+   code += "(call $test (call $i64_trunc_u_f64 (f64.const 1)))\n";
+   code += "))";
+
+   wasm_interface_impl wasm_imp(wasm_interface::vm_type::binaryen);


this needs to be the runtime that the unit tests are targeting

learnforpractice · 2018-05-04T00:22:12Z

module_interface in account_module is used for reference counting, so when no account reference to the code anymore, we can delete the code from instantiation_cache safely. //check code reference if (oldcode_replaced) { auto _it = instantiation_cache.find(old_digest); if (_it != instantiation_cache.end()) { //no account reference to this instantiated module anymore, release it if (_it->second.use_count() == 1) { instantiation_cache.erase(_it); } } } This prevent instantiation_cache from memory leak.

…

On May 4, 2018, at 4:55 AM, wanderingbort ***@***.***> wrote: @wanderingbort requested changes on this pull request. In libraries/chain/include/eosio/chain/wasm_interface_private.hpp <#2701 (comment)>: > @@ -19,6 +19,12 @@ using namespace Runtime; namespace eosio { namespace chain { + struct account_module { + digest_type digest; + std::shared_ptr<wasm_instantiated_module_interface> module_interface; This doesn't seem to be used anymore, meaning the map<account_name, account_module> account_module_map can probably become a map<account_name, digest_type> That would remove the need to change the std::unique_ptr to a std::shared_ptr as well which is nicer. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2701 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ad6ZPogrFNe5t-eOVwLEwKwFhiS3yRr6ks5tu27OgaJpZM4Tu_l3>.

learnforpractice · 2018-05-04T00:31:11Z

wasm_interface_impl::get_instantiated_module has shared_vector<char> as it’s parameter, I can not find a way about converting vector<char> to shared_vector directly. So managed_mapped_file is just for the purpose of keep the code in wasm_interface_private.hpp unchanged, but still it can be tested.

…

On May 4, 2018, at 4:59 AM, wanderingbort ***@***.***> wrote: @wanderingbort commented on this pull request. In tests/tests/misc_tests.cpp <#2701 (comment)>: > + (memory $0 1) + (export "apply" (func $apply)) + (func $i64_trunc_u_f64 (param $0 f64) (result i64) (i64.trunc_u/f64 (get_local $0))) + (func $test (param $0 i64)) + (func $apply (param $0 i64)(param $1 i64)(param $2 i64) + )====="; + + std::string code = wast_code; + code += "(call $test (call $i64_trunc_u_f64 (f64.const 1)))\n"; + code += "))"; + + wasm_interface_impl wasm_imp(wasm_interface::vm_type::binaryen); + + auto wasm = ::eosio::chain::wast_to_wasm(code); + + auto wasm_code = shared_vector<char>(alloc); shared_vector is just a vector that has a different allocator. For the purposes of your test it could just be a vector<char> which would make the inclusion of bip::managed_mapped_file seg(bip::open_or_create,"./test.db", 1024*1024); and all the related code to derive alloc unnecessary — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2701 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ad6ZPuD2D9yUfRhDtJWRPM6YQVCprKLCks5tu2_DgaJpZM4Tu_l3>.

learnforpractice · 2018-05-04T00:34:51Z

Or can someone tell me how to convert vector<char> to shared_vector<char> nice and easy, so I can remove the ugly code :).

…

On May 4, 2018, at 8:31 AM, john rok ***@***.***> wrote: wasm_interface_impl::get_instantiated_module has shared_vector<char> as it’s parameter, there is no way you can convert vector<char> to shared_vector directly. So managed_mapped_file is just for the purpose of keep the code in wasm_interface_private.hpp unchanged, but still it can be tested. > On May 4, 2018, at 4:59 AM, wanderingbort ***@***.*** ***@***.***>> wrote: > > @wanderingbort commented on this pull request. > > In tests/tests/misc_tests.cpp <#2701 (comment)>: > > > + (memory $0 1) > + (export "apply" (func $apply)) > + (func $i64_trunc_u_f64 (param $0 f64) (result i64) (i64.trunc_u/f64 (get_local $0))) > + (func $test (param $0 i64)) > + (func $apply (param $0 i64)(param $1 i64)(param $2 i64) > + )====="; > + > + std::string code = wast_code; > + code += "(call $test (call $i64_trunc_u_f64 (f64.const 1)))\n"; > + code += "))"; > + > + wasm_interface_impl wasm_imp(wasm_interface::vm_type::binaryen); > + > + auto wasm = ::eosio::chain::wast_to_wasm(code); > + > + auto wasm_code = shared_vector<char>(alloc); > shared_vector is just a vector that has a different allocator. For the purposes of your test it could just be a vector<char> which would make the inclusion of bip::managed_mapped_file seg(bip::open_or_create,"./test.db", 1024*1024); and all the related code to derive alloc unnecessary > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub <#2701 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ad6ZPuD2D9yUfRhDtJWRPM6YQVCprKLCks5tu2_DgaJpZM4Tu_l3>. >

wanderingbort · 2018-05-04T12:43:04Z

module_interface in account_module is used for reference counting, so when no account reference to the code anymore,

This is true however, you never access account_module::module_instance from an iterator or entry in the account_module_map as far as I can tell.

I can only find an access for account_module::digest which is used to find the entry instantiation_cache, decrement the reference count and potentially erase the entry.

Basically, the mechanism is sound but I don't see a reason to share ownership of wasm_instantiated_module_interface between the two maps. Only one is ever used to access that data and therefore it seems like this mechanism would still work as designed with instantiation_cache having the only ownership and using std::unique_ptr

wanderingbort · 2018-05-04T12:48:20Z

wasm_interface_impl::get_instantiated_module has shared_vector as it’s parameter,

Good catch, I did not realize that. I think its cleaner to have the storage semantics defined elsewhere. In the past, we've other places we've passed const char* buf, size_t buf_size in place of a shared_vector<char> and then made the caller extract those values from their preferred storage class before calling the method.

If you want to take that approach, I would approve of it. It adds a small amount of complexity to the caller and removes a large amount of complexity from your test code and all the called code.

learnforpractice · 2018-05-04T14:29:21Z

@wanderingbort, I think you understand why I use share_ptr, My purpose is to tracking the reference of code used by accounts since the same code may be used by more than one account and once no account reference to the code in instantiation_cache anymore, the code need to be release to prevent instantiation_cache from memory leak. Since accounts can update their code with no times limit, this bug may be used by malicious attack and paralyses the network in a few hours. Can you show me a piece of code on how to tracking the reference of code with map<account_name, digest_type>? I can only find one way: iterate through the entire map every time updating the code in account, and I think that's inefficient. I will accept your suggestion if I can understand it’s a better solution.

…

On May 4, 2018, at 8:43 PM, wanderingbort ***@***.***> wrote: module_interface in account_module is used for reference counting, so when no account reference to the code anymore, This is true however, you never access account_module::module_instance from an iterator or entry in the account_module_map as far as I can tell. I can only find an access for account_module::digest which is used to find the entry instantiation_cache, decrement the reference count and potentially erase the entry. Basically, the mechanism is sound but I don't see a reason to share ownership of wasm_instantiated_module_interface between the two maps. Only one is ever used to access that data and therefore it seems like this mechanism would still work as designed with instantiation_cache having the only ownership and using std::unique_ptr — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2701 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ad6ZPj0sHWGhbstm_0-E-ydJWCW7zbDNks5tvEzxgaJpZM4Tu_l3>.

wanderingbort · 2018-05-04T16:24:49Z

What I missed was that you were using std::shared_ptr as a basic reference count. This is a very opaque way of achieving what you want and has the downside of implying that there is shared ownership of the module when there is not.

I would recommend making the fact that you are reference counting explicit since you don't actually need shared memory ownership.

consider this patch: https://gist.github.com/wanderingbort/117d0d21b0affab98d9216f33174ac18
This is an example using std::pair that passes your test and makes the mechanism clearer in my opinion without granting shared ownership over the lifecycle of the allocated memory.

That said, if you integrate this patch I would make an explicit struct instead of using std::pair but I wanted to make my point as quickly as possible.

learnforpractice · 2018-05-04T23:06:13Z

@wanderingbort Nice work, it’s better than I thought. Patch done. I got stuck in my own mind. Also return a reference to shared_ptr is actually unsafe.

…

On May 5, 2018, at 12:25 AM, wanderingbort ***@***.***> wrote: What I missed was that you were using std::shared_ptr as a basic reference count. This is a very opaque way of achieving what you want and has the downside of implying that there is shared ownership of the module when there is not. I would recommend making the fact that you are reference counting explicit since you don't actually need shared memory ownership. consider this patch: https://gist.github.com/wanderingbort/117d0d21b0affab98d9216f33174ac18 <https://gist.github.com/wanderingbort/117d0d21b0affab98d9216f33174ac18> This is an example using std::pair that passes your test and makes the mechanism clearer in my opinion without granting shared ownership over the lifecycle of the allocated memory. That said, if you integrate this patch I would make an explicit struct instead of using std::pair but I wanted to make my point as quickly as possible. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2701 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ad6ZPuEHagTo6lVCF5Ijovqwqk7crEDlks5tvIDrgaJpZM4Tu_l3>.

spoonincode · 2018-05-09T19:07:50Z

I think after some internal discussion a couple days ago we decided this was not what we wanted to do. Two reasons: 1) forks could throb cache invalidation more than desired and 2) we actually need the cache to additionally be aware of the code's "injection version"

learnforpractice · 2018-05-10T03:51:44Z

Hmm, it’s the core part, good to hear that you guys have a better thought about improvement.

…

On May 10, 2018, at 3:08 AM, Matt Witherspoon ***@***.***> wrote: I think after some internal discussion a couple days we decided this was not what we wanted to do. Two reasons: 1) hard forks could throb cache invalidation more than desired and 2) we actually need the cache to additionally be aware of the code's "injection version" — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2701 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/Ad6ZPppfSgTHNRep61kswD5kxaJ-3PZeks5twz6mgaJpZM4Tu_l3>.

gleehokie · 2018-05-16T04:25:30Z

@spoonincode should we close this PR?

bytemaster · 2018-05-28T21:48:04Z

I would like to revisit this issue. Fork thrashing could be resolved by expiring cache on IRB rather than immediately upon changing of code. Lets resolve the merge conflicts.

spoonincode · 2018-05-29T19:16:50Z

I wonder if the garbage collector in WAVM would need to be fired for the WAVM modules to actually be freed.

learnforpractice · 2019-01-22T02:53:26Z

Sorry, I deleted my eos fork accidentally, Hope this issue will be solved soon.

wanderingbort suggested changes May 3, 2018

View reviewed changes

wanderingbort reviewed May 3, 2018

View reviewed changes

spoonincode reviewed May 3, 2018

View reviewed changes

bytemaster mentioned this pull request May 28, 2018

Resolve Conflicts & Merge Pull 2701 #3515

Closed

bytemaster added the Merge Conflict label May 29, 2018

bytemaster assigned larryk85 May 29, 2018

arhag added the Needs Revisions label May 29, 2018

larryk85 removed the Merge Conflict label May 30, 2018

learnforpractice closed this Jan 22, 2019

spoonincode mentioned this pull request Mar 22, 2019

Clean up wasm cache entries based on irreversibility & fix wavm module cleanup #6983

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak in instantiation_cache #2701

Fix memory leak in instantiation_cache #2701

learnforpractice commented May 2, 2018

learnforpractice commented May 2, 2018

learnforpractice commented May 2, 2018 •

edited

Loading

spoonincode commented May 3, 2018

wanderingbort May 3, 2018

wanderingbort May 3, 2018

spoonincode May 3, 2018

learnforpractice commented May 4, 2018 via email •

edited

Loading

learnforpractice commented May 4, 2018 via email •

edited

Loading

learnforpractice commented May 4, 2018 via email

wanderingbort commented May 4, 2018

wanderingbort commented May 4, 2018

learnforpractice commented May 4, 2018 via email •

edited

Loading

wanderingbort commented May 4, 2018

learnforpractice commented May 4, 2018 via email

spoonincode commented May 9, 2018 •

edited

Loading

learnforpractice commented May 10, 2018 via email

gleehokie commented May 16, 2018

bytemaster commented May 28, 2018

spoonincode commented May 29, 2018

learnforpractice commented Jan 22, 2019


		auto wasm = ::eosio::chain::wast_to_wasm(code);

		auto wasm_code = shared_vector<char>(alloc);

Fix memory leak in instantiation_cache #2701

Fix memory leak in instantiation_cache #2701

Conversation

learnforpractice commented May 2, 2018

learnforpractice commented May 2, 2018

learnforpractice commented May 2, 2018 • edited Loading

spoonincode commented May 3, 2018

wanderingbort May 3, 2018

Choose a reason for hiding this comment

wanderingbort May 3, 2018

Choose a reason for hiding this comment

spoonincode May 3, 2018

Choose a reason for hiding this comment

learnforpractice commented May 4, 2018 via email • edited Loading

learnforpractice commented May 4, 2018 via email • edited Loading

learnforpractice commented May 4, 2018 via email

wanderingbort commented May 4, 2018

wanderingbort commented May 4, 2018

learnforpractice commented May 4, 2018 via email • edited Loading

wanderingbort commented May 4, 2018

learnforpractice commented May 4, 2018 via email

spoonincode commented May 9, 2018 • edited Loading

learnforpractice commented May 10, 2018 via email

gleehokie commented May 16, 2018

bytemaster commented May 28, 2018

spoonincode commented May 29, 2018

learnforpractice commented Jan 22, 2019

learnforpractice commented May 2, 2018 •

edited

Loading

learnforpractice commented May 4, 2018 via email •

edited

Loading

learnforpractice commented May 4, 2018 via email •

edited

Loading

learnforpractice commented May 4, 2018 via email •

edited

Loading

spoonincode commented May 9, 2018 •

edited

Loading