Stream: git-wasmtime

Topic: wasmtime / PR #8541 Call-indirect caching: protect agains...

Wasmtime GitHub notifications bot (May 03 2024 at 18:21):

cfallin opened PR #8541 from cfallin:oob-table-index-during-prescan to bytecodealliance:main:

…ng prescan.

Call-indirect caching requires a "prescan" of a module's code section during compilation in order to know which tables are possibly written, and to count call-indirect callsites so that each separate function compilation can enable caching if possible and use the right slot range.

This prescan is not integrated with the usual validation logic (nor should it be, probably, for performance), so it's possible that an out-of-bounds table index or other illegal instruction could be present. We previously indexed into an internal data structure (table_plans) with this index, allowing for a compilation panic on certain invalid modules before validation would have caught it. This PR fixes that with a one-off check at the single point that we interpret a parameter (the table index) from an instruction during the prescan.

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68614.

No test case because this requires compilation with non-default parameters, so the usual WAST assert_invalid form can't be used in a WAST test, and requires an invalid module, so a compilation disas test can't be used. Could build out either if desired but that seems like a much larger change.

Wasmtime GitHub notifications bot (May 03 2024 at 18:21):

cfallin requested wasmtime-core-reviewers for a review on PR #8541.

Wasmtime GitHub notifications bot (May 03 2024 at 18:21):

cfallin requested alexcrichton for a review on PR #8541.

Wasmtime GitHub notifications bot (May 03 2024 at 18:21):

cfallin edited PR #8541.

Wasmtime GitHub notifications bot (May 03 2024 at 18:21):

cfallin edited PR #8541:

Call-indirect caching requires a "prescan" of a module's code section during compilation in order to know which tables are possibly written, and to count call-indirect callsites so that each separate function compilation can enable caching if possible and use the right slot range.

This prescan is not integrated with the usual validation logic (nor should it be, probably, for performance), so it's possible that an out-of-bounds table index or other illegal instruction could be present. We previously indexed into an internal data structure (table_plans) with this index, allowing for a compilation panic on certain invalid modules before validation would have caught it. This PR fixes that with a one-off check at the single point that we interpret a parameter (the table index) from an instruction during the prescan.

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=68614.

No test case because this requires compilation with non-default parameters, so the usual WAST assert_invalid form can't be used in a WAST test, and requires an invalid module, so a compilation disas test can't be used. Could build out either if desired but that seems like a much larger change.

Wasmtime GitHub notifications bot (May 03 2024 at 18:59):

alexcrichton submitted PR review:

Thanks!

Can you add a test similar to https://github.com/bytecodealliance/wasmtime/pull/8544? Basically just assert Module::new returns an error. I was thinking when writing that PR we should add support for custom CLI flags on the *.wast test suite, but that can be added orthogonally to this.

Wasmtime GitHub notifications bot (May 03 2024 at 19:59):

cfallin updated PR #8541.

Wasmtime GitHub notifications bot (May 03 2024 at 19:59):

cfallin commented on PR #8541:

Ah, right, that's the straightforward and obvious way to test this -- thanks! Added.

Wasmtime GitHub notifications bot (May 03 2024 at 19:59):

cfallin has enabled auto merge for PR #8541.

Wasmtime GitHub notifications bot (May 03 2024 at 20:38):

cfallin merged PR #8541.

Last updated: Oct 23 2024 at 20:03 UTC