diff --git a/tracing-subscriber/src/reload.rs b/tracing-subscriber/src/reload.rs index f447a5f494..22370cc45d 100644 --- a/tracing-subscriber/src/reload.rs +++ b/tracing-subscriber/src/reload.rs @@ -148,10 +148,16 @@ where #[doc(hidden)] unsafe fn downcast_raw(&self, id: TypeId) -> Option> { - // Safety: It is generally unsafe to downcast through a reload, because + // Safety: it is generally unsafe to downcast through a reload, because // the pointer can be invalidated after the lock is dropped. // `NoneLayerMarker` is a special case because it // is never dereferenced. + // + // Additionally, even if the marker type *is* dereferenced (which it + // never will be), the pointer should be valid even if the subscriber + // is reloaded, because all `NoneLayerMarker` pointers that we return + // actually point to the global static singleton `NoneLayerMarker`, + // rather than to a field inside the lock. if id == TypeId::of::() { return try_lock!(self.inner.read(), else return None).downcast_raw(id); } diff --git a/tracing-subscriber/src/subscribe/mod.rs b/tracing-subscriber/src/subscribe/mod.rs index e40e65a397..5041459866 100644 --- a/tracing-subscriber/src/subscribe/mod.rs +++ b/tracing-subscriber/src/subscribe/mod.rs @@ -1493,8 +1493,8 @@ pub struct Identity { // === impl Subscribe === #[derive(Clone, Copy)] -pub(crate) struct NoneLayerMarker; -pub(crate) static NONE_LAYER_MARKER: NoneLayerMarker = NoneLayerMarker; +pub(crate) struct NoneLayerMarker(()); +pub(crate) static NONE_LAYER_MARKER: NoneLayerMarker = NoneLayerMarker(()); /// Is a type implementing `Subscriber` `Option::<_>::None`? pub(crate) fn subscriber_is_none(subscriber: &S) -> bool @@ -1503,10 +1503,13 @@ where C: Collect, { unsafe { - // Safety: we're not actually *doing* anything with this pointer --- we - // only care about the `Option`, which we're turning into a `bool`. So - // even if the subscriber decides to be evil and give us some kind of invalid - // pointer, we don't ever dereference it, so this is always safe. + // Safety: we're not actually *doing* anything with this pointer --- + // this only care about the `Option`, which is essentially being used + // as a bool. We can rely on the pointer being valid, because it is + // a crate-private type, and is only returned by the `Subscribe` impl + // for `Option`s. However, even if the subscriber *does* decide to be + // evil and give us an invalid pointer here, that's fine, because we'll + // never actually dereference it. subscriber.downcast_raw(TypeId::of::()) } .is_some()