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.

Sign up for GitHub

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

Cap hard memory limit at 4GB #748

Merged
merged 3 commits into from
Mar 10, 2021
+35 −5
Merged

Cap hard memory limit at 4GB #748

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fbf9c17
Rename MemoryPagesValidationLimit -> MemoryPagesMaxLimit
gumb0 Mar 10, 2021
16cade6
Disallow instantiation with hard memory limit above 4GB
gumb0 Mar 10, 2021
207741f
test: Add memory.grow cases with huge hard memory limit
gumb0 Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
test: Add memory.grow cases with huge hard memory limit
@gumb0
gumb0 committed Mar 10, 2021
commit 207741f59af66cad029e16476e3be71bac94c4ce
18 changes: 18 additions & 0 deletions test/unittests/execute_test.cpp
View file
Original file line number Diff line number Diff line change
@@ -673,6 +673,7 @@ TEST(execute, memory_grow_custom_hard_limit)
{1, 1},
{15, 1},
{16, -1},
{65535, -1},
{0xffffffff, -1},
};

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

{
const auto instance_huge_hard_limit = instantiate(*module, {}, {}, {}, {}, 65536);
// For huge hard limit we test only failure cases, because allocating 4GB of memory would
// be too slow for unit tests.
// EXPECT_THAT(execute(*instance_huge_hard_limit, 0, {65535}), Result(1));
EXPECT_THAT(execute(*instance_huge_hard_limit, 0, {65536}), Result(-1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

EXPECT_THAT(execute(*instance_huge_hard_limit, 0, {0xffffffff}), Result(-1));
}

/* wat2wasm
(memory 1 16)
(func (param i32) (result i32)
@@ -734,6 +744,14 @@ TEST(execute, memory_grow_custom_hard_limit)
EXPECT_THAT(execute(*instance_max_limit, 0, {input}), Result(expected));
}

{
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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment here.

EXPECT_THAT(execute(*instance_huge_hard_limit, 0, {0xffffffff}), Result(-1));
}

/* wat2wasm
(memory (import "mod" "mem") 1 16)
(func (param i32) (result i32)