Validate constant expressions in globals initializers

circle.yml

@@ -252,8 +252,8 @@ jobs:
       - benchmark:
           min_time: "0.01"
       - spectest:
-          expected_passed: 5136
-          expected_failed: 296
+          expected_passed: 5138
+          expected_failed: 294
           expected_skipped: 6381
 
   sanitizers-macos:
@@ -270,8 +270,8 @@ jobs:
       - benchmark:
           min_time: "0.01"
       - spectest:
-          expected_passed: 5136
-          expected_failed: 296
+          expected_passed: 5138
+          expected_failed: 294
           expected_skipped: 6381
 
   benchmark:
@@ -381,8 +381,8 @@ jobs:
           expected_failed: 9
           expected_skipped: 7323
       - spectest:
-          expected_passed: 5136
-          expected_failed: 296
+          expected_passed: 5138
+          expected_failed: 294
           expected_skipped: 6381
 
 workflows:

lib/fizzy/execute.cpp

@@ -495,14 +495,9 @@ std::unique_ptr<Instance> instantiate(Module module,
     globals.reserve(module.globalsec.size());
     for (auto const& global : module.globalsec)
     {
-        // Wasm spec section 3.3.7 constrains initialization by another global to const imports only
-        // https://webassembly.github.io/spec/core/valid/instructions.html#expressions
-        if (global.expression.kind == ConstantExpression::Kind::GlobalGet &&
-            global.expression.value.global_index >= imported_globals.size())
-        {
-            throw instantiate_error{
-                "global can be initialized by another const global only if it's imported"};
-        }
+        // Constraint to use global.get only with imported globals is checked at validation.
+        assert(global.expression.kind != ConstantExpression::Kind::GlobalGet ||
+               global.expression.value.global_index < imported_globals.size());
 
         const auto value = eval_constant_expression(
             global.expression, imported_globals, module.globalsec, globals);

lib/fizzy/parser.cpp

@@ -12,6 +12,22 @@
 
 namespace fizzy
 {
+namespace
+{
+void validate_constant_expression(const ConstantExpression& const_expr, const Module& module)
+{
+    if (const_expr.kind == ConstantExpression::Kind::Constant)
+        return;
+
+    const auto global_idx = const_expr.value.global_index;
+    if (global_idx >= module.get_global_count())
+        throw validation_error{"invalid global index in constant expression"};
+
+    if (module.is_global_mutable(global_idx))
+        throw validation_error{"constant expression can use global.get only for const globals"};
+}
+}  // namespace
+
 template <typename T>
 parser_result<T> parse(const uint8_t* pos, const uint8_t* end);
 
@@ -533,11 +549,26 @@ Module parse(bytes_view input)
     if (!module.elementsec.empty() && !module.has_table())
         throw validation_error{"element section encountered without a table section"};
 
+    const auto total_global_count = module.get_global_count();
+    for (const auto& global : module.globalsec)
+    {
+        validate_constant_expression(global.expression, module);
+
+        // Wasm spec section 3.3.7 constrains initialization by another global to const imports only
+        // https://webassembly.github.io/spec/core/valid/instructions.html#expressions
+        if (global.expression.kind == ConstantExpression::Kind::GlobalGet &&
+            global.expression.value.global_index >= module.imported_global_types.size())
+        {
+            throw validation_error{
+                "global can be initialized by another const global only if it's imported"};
+        }
+    }
+
+
     if (module.funcsec.size() != code_binaries.size())
         throw parser_error{"malformed binary: number of function and code entries must match"};
 
     const auto total_func_count = module.get_function_count();
-    const auto total_global_count = module.get_global_count();
 
     // Validate exports.
     std::unordered_set<std::string_view> export_names;

test/unittests/instantiate_test.cpp

@@ -857,31 +857,6 @@ TEST(instantiate, globals_initialized_from_imported)
 
     ASSERT_EQ(instance->globals.size(), 1);
     EXPECT_EQ(instance->globals[0], 42);
-
-    // initializing from mutable global is not allowed
-    /* wat2wasm --no-check
-      (global (import "mod" "g1") (mut i32))
-      (global (mut i32) (global.get 0))
-    */
-    const auto bin_invalid1 =
-        from_hex("0061736d01000000020b01036d6f64026731037f010606017f0123000b");
-    const auto module_invalid1 = parse(bin_invalid1);
-
-    ExternalGlobal g_mutable{&global_value, true};
-
-    EXPECT_THROW_MESSAGE(instantiate(module_invalid1, {}, {}, {}, {g_mutable}), instantiate_error,
-        "constant expression can use global_get only for const globals");
-
-    // initializing from non-imported global is not allowed
-    /* wat2wasm --no-check
-      (global i32 (i32.const 42))
-      (global (mut i32) (global.get 0))
-    */
-    const auto bin_invalid2 = from_hex("0061736d01000000060b027f00412a0b7f0123000b");
-    const auto module_invalid2 = parse(bin_invalid2);
-
-    EXPECT_THROW_MESSAGE(instantiate(module_invalid2, {}, {}), instantiate_error,
-        "global can be initialized by another const global only if it's imported");
 }
 
 TEST(instantiate, start_unreachable)

test/unittests/parser_test.cpp

@@ -594,12 +594,17 @@ TEST(parser, global_single_mutable_const_inited)
     EXPECT_EQ(module.globalsec[0].expression.value.constant, 0x10);
 }
 
-TEST(parser, global_single_const_global_inited)
+TEST(parser, global_multi_global_inited)
 {
-    const auto section_contents = bytes{0x01, 0x7f, 0x00, uint8_t(Instr::global_get), 0x01, 0x0b};
-    const auto bin = bytes{wasm_prefix} + make_section(6, section_contents);
-
+    /* wat2wasm
+      (global (import "m" "g1") i32)
+      (global (import "m" "g2") i32)
+      (global i32 (global.get 1))
+    */
+    const auto bin =
+        from_hex("0061736d01000000021102016d026731037f00016d026732037f000606017f0023010b");
     const auto module = parse(bin);
+
     ASSERT_EQ(module.globalsec.size(), 1);
     EXPECT_FALSE(module.globalsec[0].type.is_mutable);
     EXPECT_EQ(module.globalsec[0].type.value_type, ValType::i32);

test/unittests/validation_test.cpp

@@ -24,6 +24,36 @@ TEST(validation, imported_function_unknown_type)
         parse(wasm), validation_error, "invalid type index of an imported function");
 }
 
+TEST(validation, globals_invalid_initializer)
+{
+    // initializing from non-existing global
+    /* wat2wasm --no-check
+      (global i32 (global.get 1))
+    */
+    const auto bin_invalid1 = from_hex("0061736d010000000606017f0023010b");
+    EXPECT_THROW_MESSAGE(
+        parse(bin_invalid1), validation_error, "invalid global index in constant expression");
+
+    // initializing from mutable global is not allowed
+    /* wat2wasm --no-check
+      (global (import "mod" "g1") (mut i32))
+      (global (mut i32) (global.get 0))
+    */
+    const auto bin_invalid2 =
+        from_hex("0061736d01000000020b01036d6f64026731037f010606017f0123000b");
+    EXPECT_THROW_MESSAGE(parse(bin_invalid2), validation_error,
+        "constant expression can use global.get only for const globals");
+
+    // initializing from non-imported global is not allowed
+    /* wat2wasm --no-check
+      (global i32 (i32.const 42))
+      (global (mut i32) (global.get 0))
+    */
+    const auto bin_invalid3 = from_hex("0061736d01000000060b027f00412a0b7f0123000b");
+    EXPECT_THROW_MESSAGE(parse(bin_invalid3), validation_error,
+        "global can be initialized by another const global only if it's imported");
+}
+
 TEST(validation, import_memories_multiple)
 {
     const auto section_contents =