Cap hard memory limit at 4GB #748

gumb0 · 2021-03-03T15:14:02Z

No description provided.

codecov · 2021-03-03T15:47:06Z

Codecov Report

Merging #748 (207741f) into master (0b41654) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #748   +/-   ##
=======================================
  Coverage   99.26%   99.26%           
=======================================
  Files          74       74           
  Lines       11448    11462   +14     
=======================================
+ Hits        11364    11378   +14     
  Misses         84       84

Flag	Coverage Δ
rust	`99.86% <ø> (ø)`
spectests	`90.55% <50.00%> (-0.11%)`	⬇️
unittests	`99.22% <100.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lib/fizzy/instantiate.cpp	`100.00% <100.00%> (ø)`
lib/fizzy/parser.cpp	`100.00% <100.00%> (ø)`
test/unittests/execute_test.cpp	`100.00% <100.00%> (ø)`
test/unittests/instantiate_test.cpp	`100.00% <100.00%> (ø)`
tools/wasi/wasi.cpp	`57.30% <100.00%> (ø)`

chfast

The "Rename MemoryPagesValidationLimit -> MemoryPagesMaxLimit" commit should go first.

chfast

I'm still considering using 0xffffffff limit value for "unlimited". Not sure this change helps. Maybe it is better to report hard limit in parser?

gumb0 · 2021-03-03T17:10:18Z

I'm still considering using 0xffffffff limit value for "unlimited".

What would it mean? Given that spec limits it at 4GB.

Not sure this change helps.

It helps to simplify memory.grow (otherwise you would need to check both instantiate hard limit and spec hard limit there).

Maybe it is better to report hard limit in parser?

What does this mean, pass it to parser instead of instantiate?

chfast · 2021-03-03T17:40:31Z

What would it mean? Given that spec limits it at 4GB.

Using 0xffffffff for the case where max value is omitted in limits.

What does this mean, pass it to parser instead of instantiate?

Make modules with max higher than 4GB invalid.

gumb0 · 2021-03-03T17:59:58Z

What would it mean? Given that spec limits it at 4GB.

Using 0xffffffff for the case where max value is omitted in limits.

This doesn't seem related, it's about replacing optional in Limits with magic value, isn't it?

What does this mean, pass it to parser instead of instantiate?

Make modules with max higher than 4GB invalid.

They are already invalid of course

fizzy/lib/fizzy/parser.cpp

Lines 260 to 262 in 56ffca7

    
           if ((limits.min > MemoryPagesValidationLimit) || 
        
               (limits.max.has_value() && *limits.max > MemoryPagesValidationLimit)) 
        
               throw validation_error{"maximum memory page limit exceeded"};

But modules with min/max higher than Fizzy's hard limit are not invalid, because we pass this limit to instantiate only.
(I think it's more spec-compliant to consider them valid, and fail only when allocation would go over the Fizzy's limit)

axic · 2021-03-09T18:19:13Z

lib/fizzy/limits.hpp

@@ -13,8 +13,8 @@ constexpr uint32_t PageSize = 65536;

 /// The maximum memory page limit as defined by the specification.
 /// It is only possible to address 4 GB (32-bit) of memory.
-constexpr uint32_t MemoryPagesValidationLimit = (4 * 1024 * 1024 * 1024ULL) / PageSize;
-static_assert(MemoryPagesValidationLimit == 65536);
+constexpr uint32_t MemoryPagesMaxLimit = (4 * 1024 * 1024 * 1024ULL) / PageSize;


Sorry for being late to the party. I think we should make the "max" and the "default" variable following the same scheme, either MemoryPagesMaxLimit/MemoryPagesDefaultLimit, or MaxMemoryPagesLimit/DefaultMemoryPagesLimit.

@chfast @gumb0 what do you think?

Sounds good, I would change to MaxMemoryPagesLimit/DefaultMemoryPagesLimit.

Fine to me too.

lib/fizzy/instantiate.cpp

axic · 2021-03-09T18:22:02Z

lib/fizzy/instantiate.cpp

@@ -174,6 +174,13 @@ std::tuple<bytes_ptr, Limits> allocate_memory(const std::vector<Memory>& module_
    static const auto bytes_delete = [](bytes* b) noexcept { delete b; };
    static const auto null_delete = [](bytes*) noexcept {};

+    if (memory_pages_limit > MemoryPagesMaxLimit)
+    {
+        throw instantiate_error{"hard memory limit cannot exceed " +


@gumb0 Is this now properly covered by spectests or you are planning to submit some cases?

It's not covered and can't be, as "hard memory limit" is not defined in any precise way in the spec, it's just an internal limit which can be arbitrary set by implementations.

It is not easy to find, but you can get this information from codecov. You need to go to single commit view -> Files, and then only select "spectests" flag.

https://codecov.io/gh/wasmx/fizzy/src/7d7bbfce98e8fca2927e539807beb8ffb4e8bac4/lib/fizzy/instantiate.cpp

It is not easy to find, but you can get this information from codecov.

I know, I went through that and listed the 10% missing cases in the chat last week. But that is only reflected if we upgrade out spectest branch, and we haven't done that in this PR :)

axic · 2021-03-09T18:28:20Z

test/unittests/execute_test.cpp

@@ -693,6 +694,12 @@ TEST(execute, memory_grow_custom_hard_limit)
        EXPECT_THAT(execute(*instance, 0, {input}), Result(expected));
    }

+    {
+        const auto instance_huge_hard_limit = instantiate(*module, {}, {}, {}, {}, 65536);
+        EXPECT_THAT(execute(*instance_huge_hard_limit, 0, {65536}), Result(-1));


What is the goal here? According to the other test, 65536 total is acceptable. Why don't we add something like instantiate with 0 and enlarge to 65536.

What is the goal here?

To test the check at high values of hard limit.

According to the other test, 65536 total is acceptable. Why don't we add something like instantiate with 0 and enlarge to 65536.

It would allocate 4GB, but I think we want to keep unit tests light-weight.
(there is a spec test that does this)

axic · 2021-03-10T17:05:51Z

test/unittests/execute_test.cpp

+        bytes memory(PageSize, 0);
+        const auto instance_huge_hard_limit =
+            instantiate(*module_imported, {}, {}, {{&memory, {1, std::nullopt}}}, {}, 65536);
+        EXPECT_THAT(execute(*instance_huge_hard_limit, 0, {65536}), Result(-1));


Well perhaps leave a comment then here that we should test for increasing by 65535 but don't want to spend too much time in unittests (to allocate 4gb) as spectests covers it.

Actually I checked and there're no spec tests that allocate 4GB, only one that sets the upper limit to 4GB (but doesn't allocate anything).

It is hard to test for it when VMs are allowed to return an error.

Added a comment here.

axic

I'm fine with this, though not 100% convinced our test cases are the best.

gumb0 requested review from chfast and axic March 3, 2021 15:30

chfast reviewed Mar 3, 2021

View reviewed changes

gumb0 force-pushed the cap-hard-mem-limit branch from 565247f to 8d3569d Compare March 3, 2021 17:16

gumb0 force-pushed the cap-hard-mem-limit branch from 8d3569d to 7d7bbfc Compare March 4, 2021 14:25

chfast approved these changes Mar 4, 2021

View reviewed changes

axic reviewed Mar 9, 2021

View reviewed changes

lib/fizzy/instantiate.cpp Show resolved Hide resolved

axic reviewed Mar 9, 2021

View reviewed changes

axic mentioned this pull request Mar 9, 2021

memory.grow 32-bit fix #747

Merged

gumb0 force-pushed the cap-hard-mem-limit branch from 7d7bbfc to ebbc24f Compare March 10, 2021 11:40

gumb0 requested a review from axic March 10, 2021 11:59

chfast approved these changes Mar 10, 2021

View reviewed changes

axic reviewed Mar 10, 2021

View reviewed changes

axic approved these changes Mar 10, 2021

View reviewed changes

axic force-pushed the cap-hard-mem-limit branch 2 times, most recently from e936c1f to ebbc24f Compare March 10, 2021 18:03

gumb0 force-pushed the cap-hard-mem-limit branch from ebbc24f to 4484678 Compare March 10, 2021 18:34

gumb0 added 3 commits March 10, 2021 19:36

Rename MemoryPagesValidationLimit -> MemoryPagesMaxLimit

fbf9c17

Disallow instantiation with hard memory limit above 4GB

16cade6

test: Add memory.grow cases with huge hard memory limit

207741f

gumb0 force-pushed the cap-hard-mem-limit branch from 4484678 to 207741f Compare March 10, 2021 18:36

axic approved these changes Mar 10, 2021

View reviewed changes

axic merged commit a1280d6 into master Mar 10, 2021

axic deleted the cap-hard-mem-limit branch March 10, 2021 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cap hard memory limit at 4GB #748

Cap hard memory limit at 4GB #748

gumb0 commented Mar 3, 2021

codecov bot commented Mar 3, 2021 •

edited

Loading

chfast left a comment

chfast left a comment

gumb0 commented Mar 3, 2021

chfast commented Mar 3, 2021

gumb0 commented Mar 3, 2021

axic Mar 9, 2021

gumb0 Mar 10, 2021

chfast Mar 10, 2021

axic Mar 9, 2021

gumb0 Mar 10, 2021

chfast Mar 10, 2021

axic Mar 10, 2021

axic Mar 9, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic Mar 10, 2021

gumb0 Mar 10, 2021

axic left a comment

Cap hard memory limit at 4GB #748

Cap hard memory limit at 4GB #748

Conversation

gumb0 commented Mar 3, 2021

codecov bot commented Mar 3, 2021 • edited Loading

Codecov Report

chfast left a comment

Choose a reason for hiding this comment

chfast left a comment

Choose a reason for hiding this comment

gumb0 commented Mar 3, 2021

chfast commented Mar 3, 2021

gumb0 commented Mar 3, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 3, 2021 •

edited

Loading