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

Surprising allocation in windows/compat.rs is a footgun for custom allocators #79118

Closed
Diggsey opened this issue Nov 16, 2020 · 5 comments
Closed
Labels
A-allocators Area: Custom and system allocators O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@Diggsey
Copy link
Contributor

Diggsey commented Nov 16, 2020

This function is the problem:

pub fn lookup(module: &str, symbol: &str) -> Option<usize> {
let mut module: Vec<u16> = module.encode_utf16().collect();
module.push(0);
let symbol = CString::new(symbol).unwrap();
unsafe {
let handle = c::GetModuleHandleW(module.as_ptr());
match c::GetProcAddress(handle, symbol.as_ptr()) as usize {
0 => None,
n => Some(n),
}
}
}

AcquireSRWLockExclusive is one of the functions utilizing this compatibility layer. However, most custom allocators indirectly rely on this function.

This means that allocators must be re-entrant, which is essentially impossible since re-entrancy detection requires thread-local storage, which in turn calls AcquireSRWLockExclusive.

For some reason I haven't been able to fathom, this does not always result in a crash, so many custom allocators which rely on TLS appear to work. However, seemingly inconsequential changes (such as changing the size of a struct) result in the program crashing with an access violation or stack overflow.

The fix is to remove this allocation and encode these strings at compile-time.

@jyn514 jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. O-windows Operating system: Windows A-allocators Area: Custom and system allocators labels Nov 17, 2020
@RalfJung
Copy link
Member

RalfJung commented Nov 22, 2020

Another fix might be to remove the compat layer, at least for locks -- which is possible once support for Windows XP is dropped (something that is discussed every now and then, and just recently came up on Zulip again I think).

@sivadeilra
Copy link

Btw, this PR: #81250 will remove the need for many of these.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 22, 2021

If we can do the conversion (to nul-terminated utf16 for the module name, and nul-terminated ascii/utf8 for the function name) at compile time, this wouldn't need to allocate at all. For the function name, this can be as simple as just a concat!(stringify!($symbol), "\0"). The other one is a bit more effort.

@sivadeilra
Copy link

We can use GetModuleHandleA instead of GetModuleHandleW. That way, the module name can be UTF-8, so can be constant.

@m-ou-se
Copy link
Member

m-ou-se commented Mar 21, 2021

#81478 fixed this.

@m-ou-se m-ou-se closed this as completed Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators O-windows Operating system: Windows T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants