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

in Windows x86, the symbol name generated by raw-dylib+undecorated are not as expected #124958

Closed
mingkuang-Chuyu opened this issue May 10, 2024 · 10 comments · Fixed by #130586
Closed
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-windows Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mingkuang-Chuyu
Copy link

mingkuang-Chuyu commented May 10, 2024

I tried this code:

#[link(name = "api-ms-win-core-synch-l1-2-0", kind = "raw-dylib", import_name_type = "undecorated")]
    extern "system" {
        pub fn WakeByAddressSingle(address: *const c_void);
    }

In WIndws x86, the symbol name that should be generated should be: __imp__WakeByAddressSingle@4
But, the symbol name of the rust generation is: __imp_WakeByAddressSingle.

When generating the lib, the symbol name should remain __imp__WakeByAddressSingle@4, just set the IMPORT_OBJECT_HEADER::NameType property to IMPORT_NAME_UNDECORATE. Then the linker will automatically convert the name to WakeByAddressSingle.

// Windows SDK(winnt.h)

typedef struct IMPORT_OBJECT_HEADER {
    WORD    Sig1;                       // Must be IMAGE_FILE_MACHINE_UNKNOWN
    WORD    Sig2;                       // Must be IMPORT_OBJECT_HDR_SIG2.
    WORD    Version;
    WORD    Machine;
    DWORD   TimeDateStamp;              // Time/date stamp
    DWORD   SizeOfData;                 // particularly useful for incremental links

    union {
        WORD    Ordinal;                // if grf & IMPORT_OBJECT_ORDINAL
        WORD    Hint;
    } DUMMYUNIONNAME;

    WORD    Type : 2;                   // IMPORT_TYPE
    WORD    NameType : 3;               // IMPORT_NAME_TYPE
    WORD    Reserved : 11;              // Reserved. Must be zero.
} IMPORT_OBJECT_HEADER;
@mingkuang-Chuyu mingkuang-Chuyu added the C-bug Category: This is a bug. label May 10, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 10, 2024
@mingkuang-Chuyu mingkuang-Chuyu changed the title Windows x86 platform, incorrect symbol names generated using rust raw-dylib undecorated Windows x86 platform, incorrect symbol names generated using rust raw-dylib+undecorated May 10, 2024
@mingkuang-Chuyu mingkuang-Chuyu changed the title Windows x86 platform, incorrect symbol names generated using rust raw-dylib+undecorated in Windows x86, incorrect symbol names generated using rust raw-dylib+undecorated May 10, 2024
@mingkuang-Chuyu mingkuang-Chuyu changed the title in Windows x86, incorrect symbol names generated using rust raw-dylib+undecorated in Windows x86, the symbol name generated by raw-dylib+undecorated are not as expected May 10, 2024
@bjorn3
Copy link
Member

bjorn3 commented May 11, 2024

Why are you using undecorated if you need the @4? import_name_type = "undecorated" tells rustc to not generate any prefixes or suffixes to the symbol. If you want them import_name_type = "decorated" (the default) is what you should use afaik.

@mingkuang-Chuyu
Copy link
Author

mingkuang-Chuyu commented May 12, 2024

Why are you using undecorated if you need the @4? import_name_type = "undecorated" tells rustc to not generate any prefixes or suffixes to the symbol. If you want them import_name_type = "decorated" (the default) is what you should use afaik.

Normally, the import_name_type attribute should not change the symbol name.

You can check the "synchronization.lib" file of the Windows SDK, the info about WakeByAddressSingle function is as follows:

dumpbin /HEADERS "C:\Program Files (x86)\Windows Kits\10\Lib\10.0.22621.0\um\x86\synchronization.lib"

image

Then we look at the disassembly code when using WakeByAddressSingle, and we can see that the symbol name is still __imp__WakeByAddressSingle@4.

image

Rust's behavior is not standard, and there are security issues.

In another lib file, there happens to be such a function. Because of Rust's non-standard behavior, they will have exactly the same symbol names. This makes it impossible for the linker to distinguish between them, and the linker will only use the first symbol.

In the following code, will rust use the _Test function or the Test function? Why is this so? This is because rust's undecorated doesn't follow the C decorated name rule!

// in rust __imp__Test
#[link(name = "DLL A", kind = "raw-dylib", import_name_type = "undecorated")]
    extern "system" {
        // extern "C" void  __stdcall _Test();
        pub fn _Test();
    }

// DLL A
#pragma comment(linker, "/export:_Test=__Test@0")
extern "C" void  __stdcall _Test()
{
    // in standard, symbol name is `__imp___Test@0` and `__Test@0`
    // in rust(undecorated), symbol name is  `__imp__Test` and `_Test`
}


// DLL B
extern "C" void __cdecl Test()
{
   // in standard, symbol name is `__imp__Test` and  `_Test`
   // Note that the name of the symbol is exactly the same as in the previous rust(undecorated) scene !!!
}

@ChrisDenton
Copy link
Member

cc @dpaoliello for this and #124956

@jieyouxu jieyouxu added A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 13, 2024
@dpaoliello
Copy link
Contributor

there are security issues

Can you please elaborate on this? If you are linking against a malicious import library or loading a malicious DLL, then name confusion doesn't provide any benefits to the attacker: they already have arbitrary read/write/execute and can use many other tricks to force their code to be executed.

When generating the lib, the symbol name should remain __imp__WakeByAddressSingle@4, just set the IMPORT_OBJECT_HEADER::NameType property to IMPORT_NAME_UNDECORATE. Then the linker will automatically convert the name to WakeByAddressSingle.

This is a fair criticism, but we'd need to make sure that using NameType produces the same behavior that Rust currently has (i.e., that we can control exactly which function name is loaded at runtime) especially in cases where GCC and MSVC disagree (see the import_name_type MCP). I wouldn't want to change this now without an MCP and a motivating example where the Rust compiler has incorrect behavior.

Rust's behavior is not standard
This is because rust's undecorated doesn't follow the C decorated name rule!

Can you please point me to documentation for this "standard" or "rule"?

Also, the intent of import_name_type is to opt-out of normal function name decoration and instead to use the modified name decoration that is documented in Rust's docs: https://doc.rust-lang.org/reference/items/external-blocks.html#the-import_name_type-key.

In the following code, will rust use the _Test function or the Test function? Why is this so?

It calls Test in DLL A.dll because you asked it to call Test without any decorations.

You have shown that Rust produces different import headers than MSVC or Clang does, but you haven't explained why this is an issue. Please remember that the goal of the raw-dylib feature in Rust is that the final binary will load a specific function from a specific DLL without the developer having to provide an import library to the linker. This is a different goal than MSVC and Clang have for their import libraries.

If you can show an example where Rust calls a function that does not match the name per the import_name_type spec in the Rust docs (conflicting symbol definitions? linker depending on SymbolName being set?) then that would be a bug that we could address.

@mingkuang-Chuyu
Copy link
Author

@dpaoliello MS decorated-names doc is here https://learn.microsoft.com/cpp/build/reference/decorated-names?view=msvc-170

From the documentation, we can see that raw-dylib+undecorated's symbol name rules are flawed.

For example, how should the following two functions be distinguished between rust?

extern "C" void  __stdcall _Test()
{
}

extern "C" void __cdecl Test()
{
}

@dpaoliello
Copy link
Contributor

@dpaoliello MS decorated-names doc is here https://learn.microsoft.com/cpp/build/reference/decorated-names?view=msvc-170

From the documentation, we can see that raw-dylib+undecorated's symbol name rules are flawed.

That document describes how to decorate names, but if you use undecorated then you are explicitly asking to NOT decorate.

We modeled the feature on the "name type" part of the PE spec, although we implemented this in Rust instead of relying on LLVM (something that I'd be willing to change, although with caution).

For example, how should the following two functions be distinguished between rust?

extern "C" void  __stdcall _Test()
{
}

extern "C" void __cdecl Test()
{
}

I think you are misunderstanding the purpose of this feature. It is not intended to be used all the time for all imports, it is very specifically designed to import functions where the exported name isn't decorated as Rust would normally expect.

So, if you can tell me how those two functions are exported, then I can tell you what #[link] attribute to use.

Examples like this and the original one in your issue are not useful - you need to show where this feature does not meet the behavior documented in Rust's docs, not where it doesn't meet your expectations. If you believe the documented behavior is incorrect, please file an MCP with the behavior that you'd recommend.

@jieyouxu jieyouxu added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed C-bug Category: This is a bug. labels Jun 4, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Jun 4, 2024

Triage: changing this to C-discussion for now, if the behavior is indeed a problem then feel free to change it back to C-bug.

@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 4, 2024
@saethlin saethlin added the O-windows Operating system: Windows label Aug 22, 2024
@ChrisDenton
Copy link
Member

Given the issue linked directly above it seems like this is an issue, at least for Firefox.

@dpaoliello
Copy link
Contributor

Given the issue linked directly above it seems like this is an issue, at least for Firefox.

Working on a fix...

@bjorn3
Copy link
Member

bjorn3 commented Sep 19, 2024

If we're at it, would it make sense to mangle the symbol name using the regular rust symbol mangling too? We do this for wasm imports too (

// * On the wasm32 targets there is a bug (or feature) in LLD [1] where the
// same-named symbol when imported from different wasm modules will get
// hooked up incorrectly. As a result foreign symbols, on the wasm target,
// with a wasm import module, get mangled. Additionally our codegen will
// deduplicate symbols based purely on the symbol name, but for wasm this
// isn't quite right because the same-named symbol on wasm can come from
// different modules. For these reasons if `#[link(wasm_import_module)]`
// is present we mangle everything on wasm because the demangled form will
// show up in the `wasm-import-name` custom attribute in LLVM IR.
//
// [1]: https://bugs.llvm.org/show_bug.cgi?id=44316
if tcx.is_foreign_item(def_id)
&& (!tcx.sess.target.is_like_wasm
|| !tcx.wasm_import_module_map(def_id.krate).contains_key(&def_id))
) and it would ensure that if you import the same symbol from two different dylibs, each call will pick the right dylib to call it from rather than picking whichever import library came first in the linker order. Or if you both define a symbol locally and import it you would still be able to call the imported one (#113050).

@jieyouxu jieyouxu added C-bug Category: This is a bug. and removed C-discussion Category: Discussion or questions that doesn't represent real issues. labels Sep 19, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 7, 2024
Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? `@ChrisDenton`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 7, 2024
Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? ``@ChrisDenton``
@bors bors closed this as completed in 60e8ab6 Nov 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 8, 2024
Rollup merge of rust-lang#130586 - dpaoliello:fixrawdylib, r=wesleywiser

Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? `@ChrisDenton`
mati865 pushed a commit to mati865/rust that referenced this issue Nov 12, 2024
Set "symbol name" in raw-dylib import libraries to the decorated name

`windows-rs` received a bug report that mixing raw-dylib generated and the Windows SDK import libraries was causing linker failures: <microsoft/windows-rs#3285>

The root cause turned out to be rust-lang#124958, that is we are not including the decorated name in the import library and so the import name type is also not being correctly set.

This change modifies the generation of import libraries to set the "symbol name" to the fully decorated name and correctly marks the import as being data vs function.

Note that this also required some changes to how the symbol is named within Rust: for MSVC we now need to use the decorated name but for MinGW we still need to use partially decorated (or undecorated) name.

Fixes rust-lang#124958

Passing i686 MSVC and MinGW build: <https://github.com/rust-lang/rust/actions/runs/11000433888?pr=130586>

r? `@ChrisDenton`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-windows Operating system: Windows T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants