Make wasm constraints configurable #8360

swatanabe-b1 · 2019-12-19T21:49:42Z

Change Description

Consensus Changes

Consensus Changes

API Changes

API Changes

Documentation Additions

Documentation Additions

…mit. This also slightly reduces the amount of data that needs to be copied on module initialization.

- OC: Use indirect access when the mutable globals need more memory than is allocated statically.

…t during module instantiation, since they are no longer correct. We've already checked them during set code anyway.

swatanabe-b1 · 2019-12-30T19:46:27Z

Depends on #8223 and EOSIO/eos-vm#144

spoonincode · 2020-03-23T18:25:53Z

libraries/chain/webassembly/eos-vm-oc/executor.cpp

+      max_pages = config.max_pages;
+   }
+   mem.reset(max_pages);
+   EOS_ASSERT(code.starting_memory_pages <= (int)max_pages, wasm_execution_error, "Initial memory out of range");


Is this line trying to detect when config.max_pages has been shrunk since the wasm was instantiated?

Why does that check need to happen here instead of a central location?

There isn't a convenient central location to put it.

It seems like the check could be wasm_interface::apply() (by way of placing starting memory pages in the wasm_cache_entry). But this would incur an additional lookup every execution which would be unfortunate.

spoonincode

There is a big problem with OC & a configurable call depth.

OC uses the native stack (unrelated to wasm stack). At the moment it ensures that no function uses more than 16KB of native stack. The idea is that 16KB*250 is ~4MB which is considerably less than default of 8MB stack across our supported platforms.

Increasing the call depth limit causes obvious problems here. Some improvements could be made, such as calculating more accurate stack usage based on the call graph. Could also try and use a separate stack for execution though that’s tricky for a few reasons and would still ultimately at the end of the day be a fixed size.

Unfortunately, since the depth limit can change transaction to transaction, it seems like OC would need to reevaluate limits each time the limit is changed.

spoonincode · 2020-03-23T18:30:53Z

libraries/chain/webassembly/eos-vm-oc/executor.cpp

+      max_call_depth = config.max_call_depth;
+      max_pages = config.max_pages;
+   }
+   mem.reset(max_pages);


Far too dangerous IMO. Could fail to acquire the resources. Also I think if the limit is large (like 2GB or something) it won't even work as-is.
Probably beyond a certain threshold (could be 33MB, could be more, could be less) OC should switch to using mprotect().

I changed it. The threshold for switching to mprotect can be configured at construction.

spoonincode · 2020-03-23T18:57:00Z

libraries/chain/webassembly/eos-vm-oc/LLVMEmitIR.cpp

 			auto tableElementType = llvm::StructType::get(context,{llvmI8PtrType, llvmI64Type});
-			defaultTablePointer = emitLiteralPointer((void*)current_prologue,tableElementType->getPointerTo(256));
+			llvm::Type* tableArrayTy = llvm::ArrayType::get(tableElementType, module.tables.defs[0].type.size.min);
+			defaultTablePointer = new llvm::GlobalVariable(*llvmModule, tableArrayTy, false, llvm::GlobalValue::ExternalLinkage, llvm::ConstantAggregateZero::get(tableArrayTy), getTableSymbolName());


does this need to be marked ExternallyInitialized?

I don't think so. The external initialization should run after the the module is fully loaded.

spoonincode · 2020-03-23T18:57:26Z

libraries/chain/webassembly/eos-vm-oc/LLVMJIT.cpp

@@ -133,7 +133,7 @@ namespace LLVMJIT
 			if(SectionName == ".stack_sizes") {
 				return stack_sizes.emplace_back(numBytes).data();
 			}
-			WAVM_ASSERT_THROW(isReadOnly);
+			//WAVM_ASSERT_THROW(isReadOnly);


what's going on here?

The wasmTable is not in a read-only section.

ah okay. This is a safety check that llvm didn't generate a data section that it expects to write to. The table should be placed in a read only section, and the assert should go back in. AFAIK just setting the GlobalVariable const param to true and using private linkage works.

I think I tried that, but eventually gave up. In order to make it const, the initialization can't be handled externally.

a35f580 has an implementation that works.

spoonincode · 2020-03-23T19:09:22Z

Can you describe what the behavior is when shrinking a limit after a wasm has been accepted? For example if the initial data max is set to 128KB, my wasm has 96KB worth of data, and then the limit is changed to 64KB.

swatanabe-b1 · 2020-03-23T19:15:35Z

The intent is that max_pages and max_call_depth are enforced on apply. The rest are enforced at set_code.

spoonincode · 2020-03-23T20:12:39Z

It seems like wasm memory initialization & memory_api need to be checktime'd now.

spoonincode · 2020-03-24T14:08:31Z

actually, I wonder if some other intrinsics are abusable at large memory sizes, 256MB, or such.

libraries/chain/webassembly/eos-vm-oc/executor.cpp

spoonincode · 2020-03-24T14:35:23Z

We need to bump OC's codegen version. I think it's probably possible to keep original code running in this new version by doing things like moving max_linear_memory_pages to the end of the control block but those changes could be done at a later time. At the moment we can just drop all version 0 on startup and start marking new ones version 1.

spoonincode · 2020-03-24T17:20:25Z

For example, something like this will run way over a 30ms deadline on EOS VM JIT, and it's only double the current memory size.

(module
 (import "env" "eosio_assert" (func $eosio_assert (param i32) (param i32)))
 (import "env" "memset" (func $memset (param i32) (param i32) (param i32) (result i32)))
 (memory 1024)
 (export "apply" (func $apply))
 (func $apply (param $0 i64) (param $1 i64) (param $2 i64)
   (drop (call $memset (i32.const 0) (i32.const 0xff) (i32.const 67108863)))
   (i32.store offset=67108860 (i32.const 0) (i32.const 0xffffffff))
   (call $eosio_assert (i32.const 1) (i32.const 0))
 )
)

Can't help but wonder if allowing a knob to control memory limits opens pandora's box on new deadline overrun attacks. Is anyone even asking for more memory?

swatanabe-b1 · 2020-03-24T19:07:56Z

I'm somewhat less concerned about deadline overruns, since this is primarily intended for private and permissioned chains. These parameters will definitely cause problems if they are set too high on a public chain.

…is too high to be safe with the default stack.

…rotect to limit the number of segments when max_pages is changed.

spoonincode · 2020-03-27T21:46:48Z

libraries/chain/webassembly/eos-vm-oc/switch_stack_linux.s

@@ -0,0 +1,13 @@
+.file	"switch_stack_linux.s"


Is this really linux only?

It's not strictly linux only, but it won't work on OSX. The code is fine on OSX, but the file format is not.

spoonincode · 2020-03-27T22:29:08Z

libraries/chain/include/eosio/chain/webassembly/eos-vm-oc/stack.hpp

+
+   template<typename F>
+   void run(F&& f) {
+      if (stack_top) {


why conditionally gate this? Why not have the altstack used all the time?

I don't have a strong opinion either way. It's mostly just to avoid an extra mmap when it isn't actually necessary.

arhag

The implementation of the new intrinsics seem to have been lost in the translation to the new host function system.

I haven't reviewed any of the changes relating to EOS VM OC (I will leave that to others). Also, I haven't yet reviewed the changes to the unit tests.

libraries/chain/CMakeLists.txt

libraries/chain/protocol_feature_manager.cpp

libraries/chain/webassembly/runtimes/eos-vm.cpp

unittests/wasm_tests.cpp

libraries/chain/include/eosio/chain/webassembly/interface.hpp

libraries/chain/webassembly/privileged.cpp

arhag

Update the fc submodule.

…ent to prevent a chain from being broken in an unrecoverable way.

…ion of validate to match.

libraries/chain/webassembly/runtimes/eos-vm.cpp

libraries/chain/include/eosio/chain/webassembly/runtime_interface.hpp

…t now.

swatanabe-b1 added 9 commits December 19, 2019 09:18

add configurable wasm limits. Not yet fully tested. Broken with OC.

44ede56

Move the wasm table into the code segment to bypass the hard-coded li…

981d87d

…mit. This also slightly reduces the amount of data that needs to be copied on module initialization.

Add tests for max_mutable_global_bytes.

d383323

- OC: Use indirect access when the mutable globals need more memory than is allocated statically.

Add tests for max_section_elements and skip all the checks in wasm-ji…

a76b244

…t during module instantiation, since they are no longer correct. We've already checked them during set code anyway.

Add tests for max_linear_memory_init

ff92bde

Add tests for max_nested_structures.

b9f5940

Add tests for max_symbol_bytes

b1d6676

Add tests for max_module_bytes and max_code_bytes.

ae50807

Update eos-vm

52a65fd

swatanabe-b1 marked this pull request as ready for review December 30, 2019 20:38

spoonincode reviewed Mar 23, 2020

View reviewed changes

spoonincode previously requested changes Mar 23, 2020

View reviewed changes

spoonincode reviewed Mar 23, 2020

View reviewed changes

spoonincode reviewed Mar 24, 2020

View reviewed changes

libraries/chain/webassembly/eos-vm-oc/executor.cpp Outdated Show resolved Hide resolved

swatanabe-b1 added 5 commits March 24, 2020 16:01

Merge remote-tracking branch 'origin/kv-database' into wasm-config

ae1edd2

Fix errors from merging

a866971

Allow eos-vm-oc to switch to another stack if the maximum call depth …

12f266a

…is too high to be safe with the default stack.

Avoid changing the layout of the control block.

77b93d8

OC: Allow the highest segment of linear memory to be resized using mp…

fcdee65

…rotect to limit the number of segments when max_pages is changed.

spoonincode reviewed Mar 27, 2020

View reviewed changes

larryk85 added 3 commits April 23, 2020 16:34

some more overlooked artifacts

0028ad6

remove extra validate

8a142dd

my goof

1e0ce4b

larryk85 changed the base branch from develop to kv-database April 24, 2020 17:00

fixed tests

9265ff0

arhag suggested changes Apr 24, 2020

View reviewed changes

larryk85 added 2 commits April 24, 2020 14:42

merge kv-database

3c4a40f

add back missing host functions

fd4090c

arhag changed the base branch from kv-database to develop April 24, 2020 19:16

arhag suggested changes Apr 24, 2020

View reviewed changes

swatanabe-b1 added 4 commits April 24, 2020 15:59

Enforce minimum values on some wasm_configuration parameters, suffici…

5dc127c

…ent to prevent a chain from being broken in an unrecoverable way.

Update fc.

09b73bd

Fix typos in description of protocol feature and update digest to match.

85a0daf

Revert changes to validate_intrinsics and update the wasm_config vers…

3b1ee9f

…ion of validate to match.

arhag reviewed Apr 24, 2020

View reviewed changes

libraries/chain/webassembly/runtimes/eos-vm.cpp Outdated Show resolved Hide resolved

swatanabe-b1 added 2 commits April 24, 2020 17:22

Remove wasm_config parameter from instantiate_module.

a4019e7

Add version to set/get_wasm_parameters packed.

5453e49

arhag reviewed Apr 24, 2020

View reviewed changes

libraries/chain/include/eosio/chain/webassembly/runtime_interface.hpp Outdated Show resolved Hide resolved

swatanabe-b1 added 4 commits April 27, 2020 09:19

Move all eos-vm-oc source files into runtimes/ and fix paths in CMake.

9307697

Remove forward declaration that is no longer needed.

cf63148

Add a couple of warnings for issues that are inconvenient to fix righ…

5e14a2c

…t now.

update eos-vm

5394a1d

spoonincode approved these changes Apr 27, 2020

View reviewed changes

arhag added 2 commits April 27, 2020 14:08

Merge branch 'develop' into wasm-config

937c3d1

Merge branch 'develop' into wasm-config

5d06cfc

arhag approved these changes Apr 27, 2020

View reviewed changes

arhag merged commit c356876 into develop Apr 27, 2020

arhag deleted the wasm-config branch April 27, 2020 23:10

heifner mentioned this pull request Sep 10, 2020

Error 3070003: Serialization Error Processing WASM, Error Details: Too many function defs #8005

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make wasm constraints configurable #8360

Make wasm constraints configurable #8360

swatanabe-b1 commented Dec 19, 2019

swatanabe-b1 commented Dec 30, 2019

spoonincode Mar 23, 2020

swatanabe-b1 Mar 23, 2020

spoonincode Mar 23, 2020

swatanabe-b1 Mar 23, 2020

spoonincode Mar 24, 2020

spoonincode left a comment

spoonincode Mar 23, 2020

swatanabe-b1 Mar 27, 2020

spoonincode Mar 23, 2020

swatanabe-b1 Mar 23, 2020

spoonincode Mar 23, 2020

swatanabe-b1 Mar 23, 2020

spoonincode Mar 23, 2020

swatanabe-b1 Mar 23, 2020

spoonincode Mar 23, 2020

spoonincode commented Mar 23, 2020

swatanabe-b1 commented Mar 23, 2020

spoonincode commented Mar 23, 2020

spoonincode commented Mar 24, 2020

spoonincode commented Mar 24, 2020

spoonincode commented Mar 24, 2020

swatanabe-b1 commented Mar 24, 2020

spoonincode Mar 27, 2020

swatanabe-b1 Mar 30, 2020

spoonincode Mar 27, 2020

swatanabe-b1 Mar 30, 2020

arhag left a comment •

edited

Loading

arhag left a comment

Make wasm constraints configurable #8360

Make wasm constraints configurable #8360

Conversation

swatanabe-b1 commented Dec 19, 2019

Change Description

Consensus Changes

API Changes

Documentation Additions

swatanabe-b1 commented Dec 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spoonincode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spoonincode commented Mar 23, 2020

swatanabe-b1 commented Mar 23, 2020

spoonincode commented Mar 23, 2020

spoonincode commented Mar 24, 2020

spoonincode commented Mar 24, 2020

spoonincode commented Mar 24, 2020

swatanabe-b1 commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arhag left a comment • edited Loading

Choose a reason for hiding this comment

arhag left a comment

Choose a reason for hiding this comment

arhag left a comment •

edited

Loading