subscriber: fix Layer::downcast_ref returning invalid references
#454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 <eliza@buoyant.io>
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 <eliza@buoyant.io>
hawkw
added
kind/bug
|
This should probably be backported to |
jonhoo
approved these changes
Nov 26, 2019
|
Looks like the nightly tests are broken due to the |
jonhoo
added a commit
to jonhoo/tracing-timing
that referenced
this pull request
Nov 26, 2019
hawkw
added a commit
that referenced
this pull request
Dec 8, 2019
Added: - `LookupSpans` implementation for `Layered` (#448) - `SpanRef::from_root` to iterate over a span's parents from the root (#460) - `Context::scope`, to iterate over the current context from the root (#460) - `Context::lookup_current`, which returns a `SpanRef` to the current span's data (#460) Changed: - Lifetimes on some new `Context` methods to be less restrictive (#460) Fixed: - `Layer::downcast_ref` returning invalid references (#454) - Compilation failure on 32-bit platforms (#462) - Compilation failure with ANSI formatters (#438) Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 4, 2020
0.2.0 (February 4, 2020) Breaking Changes: - **fmt**: Renamed `Context` to `FmtContext` (#420, #425) - **fmt**: Renamed `Builder` to `SubscriberBuilder` (#420) - **filter**: Removed `Filter`. Use `EnvFilter` instead (#434) Added: - **registry**: `Registry`, a reusable span store that `Layer`s can use a high-performance, in-memory store. (#420, #425, #432, #433, #435) - **registry**: Added `LookupSpan` trait, implemented by `Subscriber`s to expose stored span data to `Layer`s (#420) - **fmt**: Added `fmt::Layer`, to allow composing log formatting with other `Layer`s (#420) - **fmt**: Added support for JSON field and event formatting (#377, #415) - **filter**: Documentation for filtering directives (#554) Changed: - **fmt**: Renamed `Context` to `FmtContext` (#420, #425) (BREAKING) - **fmt**: Renamed `Builder` to `SubscriberBuilder` (#420) (BREAKING) - **fmt**: Reimplemented `fmt::Subscriber` in terms of the `Registry` and `Layer`s (#420) Removed: - **filter**: Removed `Filter`. Use `EnvFilter` instead (#434) (BREAKING) Fixed: - **fmt**: Fixed memory leaks in the slab used to store per-span data (3c35048) - **fmt**: `fmt::SubscriberBuilder::init` not setting up `log` compatibility (#489) - **fmt**: Spans closed by a child span closing not also closing _their_ parents (#514) - **Layer**: Fixed `Layered` subscribers failing to downcast to their own type (#549) - **Layer**: Fixed `Layer::downcast_ref` returning invalid references (#454) Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw
added a commit
that referenced
this pull request
Feb 4, 2020
# 0.2.0 (February 4, 2020) ### Breaking Changes - **fmt**: Renamed `Context` to `FmtContext` (#420, #425) - **fmt**: Renamed `Builder` to `SubscriberBuilder` (#420) - **filter**: Removed `Filter`. Use `EnvFilter` instead (#434) ### Added - **registry**: `Registry`, a `Subscriber` implementation that `Layer`s can use as a high-performance, in-memory span store. (#420, #425, #432, #433, #435) - **registry**: Added `LookupSpan` trait, implemented by `Subscriber`s to expose stored span data to `Layer`s (#420) - **fmt**: Added `fmt::Layer`, to allow composing log formatting with other `Layer`s - **fmt**: Added support for JSON field and event formatting (#377, #415) - **filter**: Documentation for filtering directives (#554) ### Changed - **fmt**: Renamed `Context` to `FmtContext` (#420, #425) (BREAKING) - **fmt**: Renamed `Builder` to `SubscriberBuilder` (#420) (BREAKING) - **fmt**: Reimplemented `fmt::Subscriber` in terms of the `Registry` and `Layer`s (#420) ### Removed - **filter**: Removed `Filter`. Use `EnvFilter` instead (#434) (BREAKING) ### Fixed - **fmt**: Fixed memory leaks in the slab used to store per-span data (3c35048) - **fmt**: `fmt::SubscriberBuilder::init` not setting up `log` compatibility (#489) - **fmt**: Spans closed by a child span closing not also closing _their_ parents (#514) - **Layer**: Fixed `Layered` subscribers failing to downcast to their own type (#549) - **Layer**: Fixed `Layer::downcast_ref` returning invalid references (#454) Fixes #515 Fixes #458 Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Currently,
Layer::downcast_rawconstructs a pointer with:This is wrong. The method's receiver is already
&self, so this codeconstructs a reference to the
&selfreference, and converts thatinto 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, thetest can still pass, as long as any pointer is returned.
Solution
This branch changes the pointer to:
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