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

Public safe fn native_cpuid::cpuid_count() technically unsafe #41

Closed
niklasf opened this issue Jan 17, 2021 · 1 comment
Closed

Public safe fn native_cpuid::cpuid_count() technically unsafe #41

niklasf opened this issue Jan 17, 2021 · 1 comment

Comments

@niklasf
Copy link
Contributor

niklasf commented Jan 17, 2021

Reviewing unsafe code in this crate, here's one more issue (but much less important, more pedantic - sorry about that).

All intrinsics in std::arch are unsafe, if only because the instruction may not be available on the current CPU. Even CPUID does not exist in all x86 and x86_64 instruction sets, so exposing the unsafe function as a safe function is technically unsound.

pub mod native_cpuid {
    pub fn cpuid_count(a: u32, c: u32) -> CpuIdResult {
        let result = unsafe { self::arch::__cpuid_count(a, c) };

        CpuIdResult {
            eax: result.eax,
            ebx: result.ebx,
            ecx: result.ecx,
            edx: result.edx,
        }
    }
}

A possible solution may be to do something equivalent to assert!(has_cpuid()) (the implementation is very interesting), or to just make the function unsafe.

A third approach would be an API to do the check only once:

struct CpuId(_priv: ());

impl CpuId {
    /// Panics if CPUID not supported.
    pub fn new() -> CpuId {
        assert!(has_cpuid());
        Self::assume_supported()
    }

    /// Safety: It is the callers responsibility to ensure that CPUID is supported.
    pub unsafe fn assume_supported() -> CpuId {
        Cpuid { _priv: () }
    }

    pub fn cpuid(&self, a: u32, c: u32) -> CpuIdResult {
        unsafe {
            // ...
        }
    }
}
@gz
Copy link
Owner

gz commented Jan 21, 2021

Hehe no worries, in fact thanks for being pedantic! This is one more dark corner of x86 :) but basically sgx environments and some ancient 32bit machines don't support cpuid.

I'm somewhat inclined to go with a just an assert/panic here rather than marking the function unsafe for ergonomics.
Reasoning would be that it's unlikely any affected 32bit machines are still relevant/using Rust today. SGX is a concern, but it has not really seen the wide-spread adoption so far. (OT: In fact, I'm willing to bet it's just another Intel feature that will become deprecated/irrelevant in the future -- and it typically requires a custom toolchain/runtime/library eco-system anyways)... Thoughts?

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

No branches or pull requests

2 participants