bastion-rs / bastion

Highly-available Distributed Fault-tolerant Runtime
https://www.bastion-rs.com
Apache License 2.0
2.78k stars 101 forks source link

make set_for_current function unsafe #346

Open kitcatier opened 1 year ago

kitcatier commented 1 year ago
Checklist

https://github.com/bastion-rs/bastion/blob/c6016a95f64965e0b9ae61c18fdfbe3437eef65f/src/bastion-executor/src/placement.rs#L20-L23 https://github.com/bastion-rs/bastion/blob/c6016a95f64965e0b9ae61c18fdfbe3437eef65f/src/bastion-executor/src/placement.rs#L71-L86 https://github.com/bastion-rs/bastion/blob/c6016a95f64965e0b9ae61c18fdfbe3437eef65f/src/bastion-executor/src/placement.rs#L214-L222 Target_OS = "windows" Hello, in some tests of your bastion_executor project, my program appeared "attempt to shift left with overflow", and the traceback indicated that the set_for_current function in this project crashed when doing a left shift operation.

extern crate bastion_executor;
use bastion_executor::placement::CoreId;
fn main() {
    let a:CoreId = CoreId { id: 771157545605903283 };
    let _ = bastion_executor::placement::set_for_current(a);
}

thread 'main' panicked at 'attempt to shift left with overflow' stack backtrace: 0: std::panicking::begin_panic_handler at /rustc/73c9eaf21454b718e7c549984d9eb6e1f75e995c/library\std\src\panicking.rs:575 1: core::panicking::panic_fmt at /rustc/73c9eaf21454b718e7c549984d9eb6e1f75e995c/library\core\src\panicking.rs:65 2: core::panicking::panic at /rustc/73c9eaf21454b718e7c549984d9eb6e1f75e995c/library\core\src\panicking.rs:115 3: bastion_executor::placement::windows::set_for_current at C:\Users\.cargo\registry\src\github.com-1ecc6299db9ec823\bastion-executor-0.4.2\src\placement.rs:216 4: bastion_executor::placement::set_for_current_helper at C:\Users\.cargo\registry\src\github.com-1ecc6299db9ec823\bastion-executor-0.4.2\src\placement.rs:180 5: bastion_executor::placement::set_for_current at C:\Users\.cargo\registry\src\github.com-1ecc6299db9ec823\bastion-executor-0.4.2\src\placement.rs:22 6: hello::main at .\src\main.rs:15 7: core::ops::function::FnOnce::call_once<void (*)(),tuple$<> > at /rustc/73c9eaf21454b718e7c549984d9eb6e1f75e995c\library\core\src\ops\function.rs:510 Target_OS = "linux" thread 'main' panicked at 'index out of bounds: the len is 16 but the index is 12049336650092238' stack backtrace: 0: rust_begin_unwind at /rustc/8a97b4812a7a46bb5206487c2455b9c5bfcbd1b9/library/std/src/panicking.rs:575:5 1: core::panicking::panic_fmt at /rustc/8a97b4812a7a46bb5206487c2455b9c5bfcbd1b9/library/core/src/panicking.rs:64:14 2: core::panicking::panic_bounds_check at /rustc/8a97b4812a7a46bb5206487c2455b9c5bfcbd1b9/library/core/src/panicking.rs:147:5 3: libc::unix::linux_like::linux::CPU_SET at /home/dl2/.cargo/registry/src/github.com-1ecc6299db9ec823/libc-0.2.139/src/unix/linux_like/linux/mod.rs:3630:9 4: bastion_executor::placement::linux::set_for_current at /home/dl2/.cargo/registry/src/github.com-1ecc6299db9ec823/bastion-executor-0.4.2/src/placement.rs:76:18 5: bastion_executor::placement::set_for_current_helper at /home/dl2/.cargo/registry/src/github.com-1ecc6299db9ec823/bastion-executor-0.4.2/src/placement.rs:44:5 6: bastion_executor::placement::set_for_current at /home/dl2/.cargo/registry/src/github.com-1ecc6299db9ec823/bastion-executor-0.4.2/src/placement.rs:22:5 7: hello_world::main at ./src/main.rs:17:13 8: core::ops::function::FnOnce::call_once at /rustc/8a97b4812a7a46bb5206487c2455b9c5bfcbd1b9/library/core/src/ops/function.rs:507:5 I think it should be marked as unsafe to remind other callers of the real legal input parameters to avoid possible errors.

o0Ignition0o commented 10 months ago

Oh you're right!

I wonder if we should check for CoreId validity before, or make it return a reuslt