Skip to content

ffi: Out-of-bound access marshaling structs by value with size and alignment less than word size #53829

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

Closed
rmacnak-google opened this issue Oct 23, 2023 · 6 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team

Comments

@rmacnak-google
Copy link
Contributor

Consider

struct S9 { 
  uint8_t a0;
  uint8_t a1;
  uint8_t a2;
  uint8_t a3;
  uint8_t a4;
  uint8_t a5;
  uint8_t a6;
  uint8_t a7;
  uint8_t a8;
};
extern void Callee(S9);
void Caller(S9* s) {
  Callee(*s);
}

on ARM64. dart:ffi marshals the struct by using two word-sized loads to fill x0 and x1. The struct has only 1-byte alignment, so the last member might be at a page boundary, so using a word load instead of byte load may trigger an access violation.

On Windows ARM64, even if the load succeds, MSVC expects the upper 56 bits of x1 to be zeroed. (As the callee, Clang does not. Both MSVC and Clang ensure these upper bits are zero as the caller.) Using an unsigned byte load during marshaling would ensure the upper bits are zero as MSVC expects.

@rmacnak-google rmacnak-google added area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi labels Oct 23, 2023
@dcharkes
Copy link
Contributor

Nice find @rmacnak-google !

This will need some massaging in kernel_to_il.cc and il_xxx.cc to make the loads/stores in various places respect bounds.

@dcharkes
Copy link
Contributor

Current results feed shows this failing or flaking on most archs/oses:

https://dart-current-results.web.app/#/filter=ffi/function_struct_by_value_out_of_bounds_test&showAll

@rmacnak-google
Copy link
Contributor Author

Oh, I should allocate a guard page explicitly instead of assuming the next page is unallocated.

copybara-service bot pushed a commit that referenced this issue Oct 30, 2023
…_bounds_test.

TEST=ci
Bug: #53829
Change-Id: I6bf32ea3e82a2ae25260931c5b0ce1814b6d9e56
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332742
Commit-Queue: Ryan Macnak <[email protected]>
Reviewed-by: Daco Harkes <[email protected]>
copybara-service bot pushed a commit that referenced this issue Oct 31, 2023
Enables running tools/test.py ffi again.

Bug: #53829
Change-Id: I155bcd9a106ea7c48700dd6fb9d4fd97815837d2
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/332762
Auto-Submit: Daco Harkes <[email protected]>
Commit-Queue: Tess Strickland <[email protected]>
Reviewed-by: Tess Strickland <[email protected]>
@a-siva
Copy link
Contributor

a-siva commented Nov 7, 2023

This is still failing on vm-fuchsia-release-x64 log

@rmacnak-google
Copy link
Contributor Author

This is expected to fail on all ABIs except IA32 and Windows X64. I have a CL in progress to handle these cases.

@a-siva a-siva added triaged Issue has been triaged by sub team P2 A bug or feature request we're likely to work on labels Nov 9, 2023
copybara-service bot pushed a commit that referenced this issue Nov 14, 2023
Switch the Windows ARM64 builds to use MSVC. Clang disagrees with itself about handling of small structs in variadic functions, allowing splitting between the last argument register and the stack as the callee but not as the caller.

TEST=ci
Cq-Include-Trybots: luci.dart.try:vm-ffi-android-debug-arm-try,vm-ffi-android-debug-arm64c-try,vm-ffi-android-release-arm-try,vm-ffi-android-release-arm64c-try,vm-ffi-qemu-linux-release-arm-try,vm-linux-release-arm64-try,vm-mac-debug-arm64-try,vm-mac-release-arm64-try,vm-win-debug-arm64-try,vm-win-release-arm64-try,vm-ffi-qemu-linux-release-riscv64-try,vm-linux-debug-ia32-try,vm-linux-release-ia32-try,vm-win-release-ia32-try,vm-linux-debug-x64-try,vm-linux-release-x64-try,vm-mac-debug-x64-try,vm-mac-release-x64-try,vm-win-debug-x64-try,vm-win-release-x64-try
Bug: #52644
Bug: #53829
Change-Id: I2fd6c40620a885479f11bb8528ca1e9df3948a2f
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/331209
Commit-Queue: Ryan Macnak <[email protected]>
Reviewed-by: Daco Harkes <[email protected]>
Reviewed-by: Siva Annamalai <[email protected]>
copybara-service bot pushed a commit that referenced this issue Nov 16, 2023
TEST=windows-x64
Bug: #53829
Change-Id: Ic7a3cd6e1e8d49a138a74a67c9d30680e91a86a6
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/336620
Commit-Queue: Ryan Macnak <[email protected]>
Reviewed-by: Daco Harkes <[email protected]>
@dcharkes
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, and the AOT and JIT backends. library-ffi P2 A bug or feature request we're likely to work on triaged Issue has been triaged by sub team
Projects
None yet
Development

No branches or pull requests

3 participants