Audit log

[jyn514]

log: GitHub, crates.io

Reverse dependencies: 4456 many, including rand, env_logger, and tokio.

I was surprised to see so many usages of unsafe in a logging crate:

Functions  Expressions  Impls  Traits  Methods  Dependency

1/1        37/49        0/0    0/0     0/2      !  log 0.4.10
0/0        0/0          0/0    0/0     0/0      ?  ├── cfg-if 0.1.10
0/0        0/15         0/0    0/0     0/2      ?  └── sval 0.5.1

1/1        37/64        0/0    0/0     0/4 

[danielhenrymantilla]

Okay, I have skimmed their code, and there are two reasons for the unsafe:

• transmuting between a usize and a #[repr(usize)] field-less enum (the log level); quite benign, but still, a wrong discriminant causes UB rather than a mismatched level of logging, so maybe we could aim for refactoring the code into something that leads to the latter:

  • e.g., using a module-namespaced set of new-typed constants rather than an enum: the behavior when matching on that enum is then never-exhaustive, and so forces users to handle it with default-branches, that on the current design, is as if they had unsafe { ::std::hint::unrechable_unchecked() }.

• type erasure and downcasting (https://github.com/rust-lang/log/blob/d54317e47a46024fdac3b05fdfcaebf243c38c9b/src/kv/value/internal/mod.rs#L142-L181). Which, although quite more dangerous, seems well implemented (it's mainly a specialized Any). My only concern, however, is that there could be safe constructors that could be used elsewhere in the crate, mainly:

  • Erased::<T>::new_unchecked<T>() (where T : Sized) is a safe constructor, so it ought to be exposed as such, removing unsafe from callers, and thus improving local reasoning;

  • As the code points out, when U : Unsize<T> (a.k.a. "a slim pointer to a U can be fattened into a pointer to a(n unsized) T", e.g., U : Debug and T = dyn Debug), the constructor is safe too. So, although no non-unsafe generic constructor can be offered without that nightly-only trait bound, the idea is still applicable with concrete Ts (e.g. T = dyn Debug).

    Either through a constructor macro:

    macro_rules! new_erased {(
        $expr:tt as &$U:ty as &$T:ty
    ) => ({
        let it: &$U = $expr;
        // Optional: ensure `$T : 'static + !Sized`
        const_assert!(::core::mem::size_of::<&'static $T>() > ::core::mem::size_of::<usize>());
        unsafe { Erased::<$T>::new_unchecked::<$U>(it as &$T) }
    })}

    Or by exposing constructors for their two actually used Ts (dyn Debug and dyn Fill).

    impl<'v> Erased<'v + dyn Debug> {
        fn new<U : Debug + 'static> (value: &'v U) -> Self
        {
            new_erased!(value as &U as &dyn Debug)
        }
     }

---

it's mainly a specialized Any

This actually makes me think that it should be possible to remove all the Erased-related unsafe by replacing:

• Erased<'v, dyn Any> with &'v dyn Any,

• Erased<'v, dyn Debug> by &'v dyn DebugAndAny where trait DebugAndAny : Debug + Any {} impl<T : Debug + Any> DebugAndAny for T {},

• Erased<'v, dyn Fill> by &'v dyn FillAndAny where ...

[jyn514]

They don't want to remove the transmute for performance reasons :/ rust-lang/log#392 (comment)

[Shnatsel]

Perhaps the "new-typed const" idea is worth pursuing then.

@danielhenrymantilla could you open a PR to convert the type erasure to safe code? I admit this is a bit over my head, so I cannot follow up on this myself.

[jyn514]

e.g., using a module-namespaced set of new-typed constants rather than an enum: the behavior when matching on that enum is then never-exhaustive, and so forces users to handle it with default-branches, that on the current design, is as if they had unsafe { ::std::hint::unrechable_unchecked() }.

This has the same performance penalty as an unreachable in the match would, I'm not sure I understand the benefit.