sadness-generator: `make_sad`'s iffy unsafety and return type

[vi]

make_sad's signature is

pub unsafe fn make_sad(self)

I see two problems in it:

1. unsafe justification is "This is not safe. It intentionally crashes.". As far as I know, crashing (especially in a controlled way) is not unsafe, as it does not lead to undefined behaviour (unless StackOverflow variant can somehow not lead to immediate abort or Segfault can fails to produce a segfault (e.g. address 0 is actually mapped. In that case it is advisable to fix it to produce reliable segfault e.g. creating a guard page and accessing it) ). If it is not possible for all the cases, at least most of the free-standing functions should be safe to use and remaining unsafe markers would tell users which function may fails to produce a clean, controllable crash.

2. Return type of the function is (). Should it be ! instead?

I suppose signature of the function should be like std::process::abort.

[Jake-Shadle]

1. I guess agree to disagree here? It's not about undefined behavior, it's about communicating to the user that in all cases, except 2, unsafe code is involved, and that code is intentionally "incorrect" to induce a crash, putting such code behind a safe signature would be misleading at best. As for reliably segfault code, aving null mapped is a niche concern IMO, but I don't particularly care about the implementation as long is is 100% reliable on all targets supported by this lib.
2. ! is nightly only, once it is stable all of the function can be made to use it instead.

[vi]

2. ! is nightly only

As far as I remember, ! was used in function signatures even before ! was promoted to a type. It works on stable.

Option<!> or Result<!,!> or other tricks with ! do indeed need Nightly.

[vi]

it's about communicating to the user that in all cases, except 2, unsafe code is involved

I expect most of those abort functions to be safe wrappers around unsafe code. For example, somebody may be interested in having a way to cause segmentation fault in production (to test recovery code) while having a guarantee that it won't execute arbitrary code from memory instead (leading to a security issue instead of a controlled crash).

intentionally "incorrect" to induce a crash

I think all (or most) of crash methods exposed by the crate can be done without "incorrect" code.

putting such code behind a safe signature would be misleading at best

Indeed, if the code is incorrect (unsound), as current segmentation fault code is, then safe signature would be misleading. But unsafe functions are supposed to have specific documentation section of ways of calling the function soundly. For today's unsound sadness_generator::raise_segfault it may be something like that:

This function tries to invoke segmentation fault by accessing a null pointer.
Note that current implementation relies on undefined behaviour (casting null pointer to a reference), so actual results may vary depending on compiler, platform and optimisation level.
Additionally the function may fail to abort the process if zero address is actually mapped to a readable memory.

Maybe at least std::ptr::read_volatile should be used instead of a cast to reference?

My version of a simplified segv function would be:

pub fn raise_segfault() -> ! {
    let p : *mut u8 = std::ptr::null_mut();
    unsafe { p.write_volatile(p.read_volatile()); }
    // Fallback for the case when zero address is actually readable and writable
    std::process::abort()
}

I expect it to be reasonably sound, except of when address zero is mapped to some tricky non-memory thing that affects CPU or memory in a weird way.

... as long is is 100% reliable on all targets supported by this lib

Relying on undefined behaviour is not 100% reliable and may break when some clever optimisation is added to rustc / LLVM. Failure mode may involve aborting process in a wrong way, just continuing execution or even a security issue.

Also target support should be documented explicitly, maybe with compile_error!()s for unsupported targets.

[Jake-Shadle]

As far as I remember, ! was used in function signatures even before ! was promoted to a type. It works on stable.

I must have been tired when I made that decision since I knew ! was usable as a function return since eg libc does it, but then when I was originally going to add it the big unstable banner at the top of the Never documentation threw me off. Oops.