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.

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

Preserve locals before entering control flow during compilation #945

Merged
merged 36 commits into from
Mar 8, 2024

Conversation

Robbepop
Copy link
Member

@Robbepop Robbepop commented Mar 3, 2024

This supposedly fixes #934.

This is still in draft due to the following reasons and ToDos:

  • Adjust all translation tests that are failing due to changes in the Wasmi codegen and check if they are still valid.
  • Add more translation tests:
    • Test for loop control flow.
      • I couldn't find a single test case that was invalid before these changes but valid after. My theory is that for loop control flow structures local preservation is not needed because it is not possible to conditionally skip local manipulation without using block or if within the loop body somewhere. Someone please proof me wrong.
    • Tests with more inputs encoding CopySpanNonOverlapping and CopyManyNonOverlapping.
    • Tests with input parameters for block and if.
    • Add a test that triggers usage of the large stack mode.
  • Improve efficiency of the WIP implementation introduced in this PR.
    • The current WIP implementation is very wasteful with memory allocations and the old data structures to manage the locals on the stack are not ideal for their current job. I already know how to fix both problems but the work needs to be done prior to merging to avoid creating new attack surfaces.
    • Avoid unnecessary memory allocations.
    • Introduce new data structure to efficiently handle local variables on the compilation stack.
    • Add len_locals to Providers stack to act as a fast bail out condition for the common case when no locals need to be preserved.
    • Preserve locals in reserve order of their appearance to avoid a non-critical attack vector in the inline mode of preservation. It is non-critical since the inline mode is used for very limited stack sizes only.
    • Replace the BTreeMap in preserve_all_locals_inplace with an ArrayVec and linear search. This avoids heap allocations and won't introduce an attack vector since preserve_all_locals_inplace is guaranteed to operate on very small sets of values.
  • Merge copy instructions resulting from the new codegen introduced in this PR.
    • This is an optimization that can easily be applied. Although given that it is only an optimization we should first make sure that everything is correct and working.
    • This uses the fact that preservation register slots are usually adjacent to each other and thus we can easily use copy2 or copy_many instructions instead of having multiple copy instructions for each preserved local.
  • Conduct benchmarks to show that the new Wasmi codegen does not significantly regress performance.
    • Performance benchmarks conducted so far locally show no significant impact.
  • Run the differential fuzzer in order to see if the changes introduced in this PR are actually generally correct.

Follow-Up PRs

  • The new codegen might result in transitive copies which we can eliminate.
  • Find out what causes the new memory-ouf-of-bounds access problems with execution of the ffmpeg.wasm binary.
    • This could either be a correctness issue in this PR or a hint to yet another unrelated bug.

Robbepop added 11 commits March 1, 2024 09:34
This implements naive local preservation in order to see if this fixes the problems with ffmpeg.wasm. This is not efficient during compilation, introduces new attack surfaces at compilation and produces inefficient code.

After asserting that this indeed fixes the issues without major performance loss the following needs to happen:

1. We need a new data structure for locals on the compilation stack to efficiently cope with the fact that all locals on the stack need to be preserved frequently, not just a single local.
2. We need to find a way to reduce dynamic allocations in FuncTranslator::preserve_locals.
3. We need to merge copies whenever possible. Since preservation register indices are contiguous we should be able to encode fused copy instructions instead of many single copy instructions.
Seems like the fixes applied to the tests as expected.
Robbepop added 15 commits March 3, 2024 20:32
While coming up with test cases I couldn't find a single one that would have invalid behavior before these changes. This is because with only a loop control flow structure there is no way to skip certain parts without using block or if control flow structures within the loop body. Someone please proof me wrong. Getting rid of as many local preservations is good for performance if not needed.
This is to unify reusable utility buffers of the Wasmi translator.
This is probably not needed but an additional safety guard
This is so that the caller can use its encoded root instruction index for notification of preservation in a future commit.
Relax its allowed copy instructions since this is going to be needed with fused copies of local preservation instructions.
This fixes a bug with RegisterSpanIter going out of bounds when trying to create a register span that starts with the initial preservation register before its index is being defragmented which caused an i16 integer overflow.

Note: we could have also altered the implementation of RegisterSpanIter, however, this would have decreased its efficiency eventually which was less desirable.
This removes a potential attack vector with a malicous Wasm file having lots of values on the compilation stack and a series of blocks or ifs that each have to preserve all locals on the compilation stack.
In order to further improve the attack vector resistance we also need to preserve locals on the compilation stack in reverse order of their appearance, e.g. as the compilation stack would have popped from.
This commit also contains some needed cleanups.
Robbepop added 5 commits March 6, 2024 23:09
Since this method is only called for small heaps we can get away by using an ArrayVec and linear search instead of using a BTreeMap.
This softens certain non-critical attack vectors on the small stack mode implementation.
@Robbepop Robbepop marked this pull request as ready for review March 8, 2024 11:08
@Robbepop
Copy link
Member Author

Robbepop commented Mar 8, 2024

Benchmarks performed locally show that performance does NOT significantly regress for both translation and execution with the changes introduced in this PR.

@Robbepop
Copy link
Member Author

Robbepop commented Mar 8, 2024

Unfortunately I cannot run the Wasmi fuzzer due to some weird nightly related issue with some dependencies:

error[E0635]: unknown feature `stdsimd`
  --> /Users/robin/.cargo/registry/src/index.crates.io-6f17d22bba15001f/ahash-0.8.6/src/lib.rs:99:42
   |
99 | #![cfg_attr(feature = "stdsimd", feature(stdsimd))]

Note that ahash is not imported from within any Wasmi dependency. So this might be related to either cargo-fuzz, Wasmtime or the outdated WASI dependencies. However, building Wasmi WASI standalone on nightly works fine.

@Robbepop
Copy link
Member Author

Robbepop commented Mar 8, 2024

I just clarified here that I am going to merge this PR as is and will start with further investigation on the ffmpeg.wasm issues until fixed entirely.

@Robbepop Robbepop changed the title WIP: Preserve locals before entering control flow during compilation Preserve locals before entering control flow during compilation Mar 8, 2024
@Robbepop Robbepop merged commit 8a1066b into master Mar 8, 2024
16 checks passed
@Robbepop Robbepop deleted the rf-fix-conditional-local-set branch March 8, 2024 14:17
@Robbepop
Copy link
Member Author

Robbepop commented Mar 8, 2024

To summarize: The fix implemented in this PR was relatively straight forward to implement. The majority of the work in this PR was focused on ensuring quality of the code and to make the new code resist malicious attackers and making it scalable for the new demands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wasmi versions 0.32.0-beta.6 cannot run ffmpeg.wasm correctly
1 participant