fix: marshal every bool-returning P/Invoke return as 1-byte (I1)#13
Merged
Conversation
.NET's default `bool` P/Invoke return marshals as UnmanagedType.Bool, reading 4 bytes (EAX). The Binary Ninja core ABI returns a 1-byte C++ `bool` in AL, and the x64 ABI leaves bits 8..63 of the return register UNDEFINED for a sub-64-bit return. A default bool return therefore reads 3 garbage bytes and can flip a `false` to `true`. Proven empirically with an isolation probe: a native function returning 0xFFFFFF00 (AL=0, upper bytes 0xFF) reads `True` under the default and `False` under UnmanagedType.I1. Latent on the installed core (it happens to zero-extend AL), but the binding targets the upstream ABI, not this install, so a different core build (MSVC version/flags) could leave garbage and surface the bug. Fix: add `[return: MarshalAs(UnmanagedType.I1)]` to all 700 bool-returning P/Invoke stubs (applied via a byte-exact transform that preserves LF line endings; 700 insertions, 0 deletions). I1 matches the existing codebase convention for 1-byte bool (struct fields use [MarshalAs(UnmanagedType.I1)]). Like the round-12 scalar-UTF8 fix, these are generated stubs; the durable fix belongs in the upstream generator (emit the return marshal). Build 0/0; e2e 1428 passed / 0 failed / 3 skipped (new BoolReturnMarshaling guard with deterministic Metadata.IsSigned/IsUnsignedInteger + Function/BasicBlock bool assertions).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
.NET's default
boolP/Invoke return marshals asUnmanagedType.Bool(reads 4 bytes / EAX). The Binary Ninja core ABI returns a 1-byte C++boolin AL, and the x64 ABI leaves bits 8..63 of the return register UNDEFINED for a sub-64-bit return. A default bool return therefore reads 3 garbage bytes and can flip afalsetotrue.Proven empirically with an isolation probe ? a native function returning
0xFFFFFF00(AL=0, upper bytes 0xFF):True(wrong)[return: MarshalAs(UnmanagedType.I1)]?False(correct)It is latent on the installed core (it happens to zero-extend AL ? e2e passes), but the binding targets the upstream ABI, not this install; a different core build (MSVC version/flags) could leave garbage and surface it across all 700 bool returns.
Fix
Add
[return: MarshalAs(UnmanagedType.I1)]to every bool-returning P/Invoke stub (all 700, inFunction/). Applied via a byte-exact transform that preserves LF line endings ? diff is 700 insertions, 0 deletions.I1matches the existing codebase convention for 1-byte bool (struct fields use[MarshalAs(UnmanagedType.I1)]).Like the round-12 scalar-UTF8 fix, these are generated stubs ? the durable fix belongs in the upstream generator.
Validation
BoolReturnMarshalingguard with deterministicMetadata.IsSigned/IsUnsignedInteger+Function/BasicBlockbool assertions).Marked as a fix (functional correctness: non-deterministic bool returns), not a refactor.