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

Segfault when using downcast_ref through Layer #453

Closed
jonhoo opened this issue Nov 26, 2019 · 0 comments · Fixed by #454
Closed

Segfault when using downcast_ref through Layer #453

jonhoo opened this issue Nov 26, 2019 · 0 comments · Fixed by #454
Labels
crate/subscriber Related to the `tracing-subscriber` crate kind/bug Something isn't working

Comments

@jonhoo
Copy link
Contributor

jonhoo commented Nov 26, 2019

Bug Report

Version

tracing 0.1.10
tracing-core 0.1.7
tracing-subscriber 0.2.0-alpha.1

Description

The following program segfaults:

use std::sync::{mpsc, Mutex};
use tracing_subscriber::{registry::Registry, Layer};

struct X(Mutex<mpsc::Receiver<()>>);
impl<S> tracing_subscriber::Layer<S> for X
where
    S: tracing::Subscriber,
    Self: 'static,
{
}

fn main() {
    let (tx, rx) = mpsc::channel();
    let d = tracing::Dispatch::new(X(Mutex::new(rx)).with_subscriber(Registry::default()));
    let mutex = &d.downcast_ref::<X>().unwrap().0;
    mutex.lock().unwrap().try_recv().unwrap_err();
    drop(tx);
}

There is no unsafe in this code, so it should at least not segfault. I believe the program should run even correctly (as in, no unwrap call should fail) given the documented behavior of downcast_ref.

jonhoo added a commit to jonhoo/tracing-timing that referenced this issue Nov 26, 2019
@hawkw hawkw added crate/subscriber Related to the `tracing-subscriber` crate kind/bug Something isn't working labels Nov 26, 2019
hawkw added a commit that referenced this issue Nov 26, 2019
These tests failed to catch issue #453 because they never actually
dereference the ref created by `downcast_ref`. Therefore, if an invalid
pointer is returned, the test can still pass, as long as _any_ pointer
is returned. This is incorrect.

This commit changes the tests to try and access data through the
returned references.

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this issue Nov 26, 2019
Currently, `Layer::downcast_raw` constructs a pointer with:
```rust
&self as *const _ as *const ()
```
This is wrong. The method's receiver is already `&self`, so this code
constructs a reference _to_ the `&self` reference, and converts _that_
into a pointer. Since this is not the actual memory address of the
layer, this is invalid.

This commit changes the pointer to:
```rust
self as *const _ as *const ()
```
Now, the returned pointer is valid.

Fixes #453

Signed-off-by: Eliza Weisman <[email protected]>
hawkw added a commit that referenced this issue Nov 26, 2019
)

## Motivation

Currently, `Layer::downcast_raw` constructs a pointer with:
```rust
&self as *const _ as *const ()
```
This is wrong. The method's receiver is already `&self`, so this code
constructs a reference _to_ the `&self` reference, and converts _that_
into a pointer. Since this is not the actual memory address of the
layer, this is invalid.

We didn't catch this because the tests for downcasting layers are
also wrong: they never actually dereference the ref created by 
`downcast_ref`. Therefore, if an invalid pointer is returned, the 
test can still pass, as long as _any_ pointer is returned.

## Solution

This branch changes the pointer to:
```rust
self as *const _ as *const ()
```
Now, the returned pointer is valid.

In addition, it changes the tests  to try and access data through the
returned references. The new tests fail with the current master 
implementation of `downcast_ref`, which is correct.

Fixes #453
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate/subscriber Related to the `tracing-subscriber` crate kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants