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

Can't build with no_std on Windows because of BCryptGenRandom import #247

Closed
not-matthias opened this issue Jan 7, 2022 · 3 comments · Fixed by #248
Closed

Can't build with no_std on Windows because of BCryptGenRandom import #247

not-matthias opened this issue Jan 7, 2022 · 3 comments · Fixed by #248
Milestone

Comments

@not-matthias
Copy link

My situation: I'm building a kernel driver and decided to use this crate. When I was building my driver, I noticed a weird import: BCryptGenRandom.

I managed to track down the issue: The import comes from the getrandom crate. It's used by ahash, which is used by hashbrown which is used by iced-x86. The problem is, that ahash doesn't hide the getrandom dependency behind a feature flag, so it's always used even though you might have enabled the compile-time-rng feature. See: https://github.com/tkaitchuck/aHash/blob/e77cab8c1e15bfc9f54dfd28bd8820c2a7bb27c4/src/random_state.rs#L95-L104
They added a runtime-rng feature, but that's only available in the simplification branch.

And this is the issue. Because getrandom always detects windows as platform, it'll import BCryptGenRandom, even though we are inside a kernel driver where we don't have access to bcrypt.dll.

How to fix (in this library):

Hashbrown has a feature that enables the compile time rng. Once the next version of ahash/hashbrown has been released, you could either enable the compile time feature by default or forward it to the user.

How to fix (on your own):

Add this to your Cargo.toml to use the updated version of aHash where getrandom is disabled:

[patch.crates-io]
# Hashbrown already imports this with `default-features=true`
ahash = { git = "https://github.com/tkaitchuck/aHash", branch = "simplification", features = ["compile-time-rng"] }

You also need to clone this repository and manually enable the compile time rng for hashbrown in src\rust\iced-x86\Cargo.toml:

hashbrown = { version = ">=0.9.0, <100.0.0", optional = true, default-features = false, features = ["ahash-compile-time-rng"] }

References

@wtfsck
Copy link
Member

wtfsck commented Jan 7, 2022

The hashbrown crate is optional and only needed by serde or block_encoder features. However it's enabled in Cargo.toml if no_std is used because I don't think it's possible to say 'Use hashbrown if no_std and (serde or block_encoder)'.

I think one easy workaround, assuming you don't need serde or block_encoder, is we could add a new feature, eg. no_std2 that would be the same as no_std but wouldn't include hashbrown. Would that work?

@not-matthias
Copy link
Author

Unfortunately, I need to use block_encoder.

And there's no need for a second feature flag, because rustc doesn't compile getrandom if it's not used. So if you don't use serde or block_encoder but have no_std enabled, you won't have the import.

@wtfsck
Copy link
Member

wtfsck commented Jan 8, 2022

I just checked and block_encoder only uses HashMap once, in a field called to_instr. I think it could be possible to convert this to a Vec and then use binary search. The init code is here:

// There must not be any instructions with the same IP, except if IP = 0 (default value)
this.to_instr = HashMap::with_capacity(instr_count);
for info in &this.blocks {
for instr in &info.1 {
let orig_ip = instr.borrow().orig_ip();
if this.to_instr.get(&orig_ip).is_some() {
if orig_ip != 0 {
return Err(IcedError::with_string(format!("Multiple instructions with the same IP: 0x{:X}", orig_ip)));
}
this.has_multiple_zero_ip_instrs = true;
} else {
let _ = this.to_instr.insert(orig_ip, instr.clone());
}
}
}
if this.has_multiple_zero_ip_instrs {
let _ = this.to_instr.remove(&0);
}

If this can be converted into an efficient Vec impl, we can remove the hashbrown dep from block_encoder.

Adding all instructions to a new vec. Sorting it. Verifying that there are no dupes (except ip==0) should work.

Block encoder perf would have to be checked as well to make sure it's still acceptable.

@wtfsck wtfsck added this to the v1.16.0 milestone Jan 8, 2022
not-matthias added a commit to not-matthias/amd_hypervisor that referenced this issue Jan 13, 2022
- Replaced jump with a long jump that doesn't use `mov rax`
- Updated trampoline setup
- Fixed `iced-x86` compilation. See: icedland/iced#247
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants