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

Calling deref on HSTRING literal macro causes undefined behaviour #3534

Open
RuairidhWilliamson opened this issue Mar 9, 2025 · 2 comments · May be fixed by #3535
Open

Calling deref on HSTRING literal macro causes undefined behaviour #3534

RuairidhWilliamson opened this issue Mar 9, 2025 · 2 comments · May be fixed by #3535
Labels
question Further information is requested

Comments

@RuairidhWilliamson
Copy link

Summary

Hi
When testing the windows_strings::h!() macro with miri it flags undefined behavior when calling deref() on the &HSTRING (ie getting the &[u16] from the &HSTRING).

fn main() {
    let my_hstr = windows_strings::h!("foo");
    let my_slice: &[u16] = &my_hstr;
    println!("{my_hstr} {my_slice:?}");
}

Miri outputs this

error: Undefined Behavior: out-of-bounds pointer use: expected a pointer to 32 bytes of memory, but got alloc101 which is only 24 bytes from the end of the allocation
   --> /home/ru/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mut_ptr.rs:288:57
    |
288 |         if self.is_null() { None } else { unsafe { Some(&*self) } }
    |                                                         ^^^^^^ out-of-bounds pointer use: expected a pointer to 32 bytes of memory, but got alloc101 which is only 24 bytes from the end of the allocation
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
    = note: BACKTRACE:
    = note: inside `std::ptr::mut_ptr::<impl *mut windows_strings::hstring_header::HStringHeader>::as_ref::<'_>` at /home/ru/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ptr/mut_ptr.rs:288:57: 288:63
    = note: inside `windows_strings::HSTRING::as_header` at /home/ru/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/windows-strings-0.3.1/src/hstring.rs:61:18: 61:33
    = note: inside `<windows_strings::HSTRING as std::ops::Deref>::deref` at /home/ru/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/windows-strings-0.3.1/src/hstring.rs:69:31: 69:47
note: inside `main`
   --> src/main.rs:3:28
    |
3   |     let my_slice: &[u16] = &my_hstr;
    |                            ^^^^^^^^

I believe this is due to the fact the h!() macro uses a different HSTRING_HEADER struct to the expected HStringHeader. The difference in these structs is appears to be because the macro does not partake in reference counting, it omits the latter reference counting fields and sets a flag to indicate that.
This means when HSTRING::as_header is called it converts the inner pointer to a reference. I believe this is undefined behavior because the reference does not point to a full HStringHeader.
I will note that this is a private method and private type, all the users of this method correctly check the flags before accessing the fields that may or may not be present, so as far as I can tell there is no way to use this incorrectly in a program.

My suggestion would be to change the HSTRING_HEADER struct to include the missing fields. To avoid exposing RefCount we can use i32.

#[doc(hidden)]
#[repr(C)]
pub struct HSTRING_HEADER {
    pub flags: u32,
    pub len: u32,
    pub padding1: u32,
    pub padding2: u32,
    pub ptr: *const u16,
    // unused because refcounting is disabled for hstring literals
    pub count: i32, 
    pub buffer_start: u16,
}

From my testing this change fixes the issue and miri no longer flags undefined behaviour. I'm happy to create a pull request for this.

Kind regards,
Ruairidh

Crate manifest

[package]
name = "winstring"
version = "0.1.0"
edition = "2024"

[dependencies]
windows-strings = "0.3.1"

Crate code

fn main() {
    let my_hstr = windows_strings::h!("foo");
    let my_slice: &[u16] = &my_hstr;
    println!("{my_hstr} {my_slice:?}");
}
@RuairidhWilliamson RuairidhWilliamson added the bug Something isn't working label Mar 9, 2025
@kennykerr kennykerr added question Further information is requested and removed bug Something isn't working labels Mar 10, 2025
@kennykerr
Copy link
Collaborator

Interesting. Have you tried #3530? I'm not sure whether that change would satisfy the analysis done by Miri.

@kennykerr
Copy link
Collaborator

I took a quick look and sure enough it repros as follows:

cargo miri test -p test_strings --test literals

The fix seems reasonable but I'd like to get some miri testing as part of the build if we're going to fix stuff like this. So I'll go ahead and open a PR with a new workflow to cover this and you can take a look and let me know what you think.

@kennykerr kennykerr linked a pull request Mar 10, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants