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

Replace owning_ref #77

Closed
DieracDelta opened this issue Aug 2, 2022 · 7 comments · Fixed by #78
Closed

Replace owning_ref #77

DieracDelta opened this issue Aug 2, 2022 · 7 comments · Fixed by #78

Comments

@DieracDelta
Copy link

DieracDelta commented Aug 2, 2022

Owning ref is (1) seemingly unmaintained since last commit was 2 years ago and (2) unsound. Something else should probably be used.

@AbhijithGanesh
Copy link

True, this issue has popped up here in LibP2P and other crates which uses this prometheus client

@AbhijithGanesh
Copy link

Can I work on patching the issue ? @mxinden

@AbhijithGanesh
Copy link

The Owning_Ref library is used at the following files:

Histogram relevant files use this module, they cannot be used due to Security reasons! This needs to be patched where the usage of Owning_Ref is dropped!

@DieracDelta
Copy link
Author

In other issues I see this being recommended. Haven't had a chance to look too deeply, but perhaps that is a suitable replacement?

@mxinden
Copy link
Member

mxinden commented Aug 4, 2022

Can I work on patching the issue ? @mxinden

Yes, for sure. Help is very much appreciated.

In other issues I see this being recommended. Haven't had a chance to look too deeply, but perhaps that is a suitable replacement?

Can you tell whether it suffers the same unsoundness issues that owning_ref does?


We need owning_ref in wrapper style metrics (e.g. Family), which internally use a Mutex though want to give users access to a reference of the underlying wrapped metric (e.g. Counter). Given that this reference depends on the MutexGuard, we need to ensure that the MutexGuard lives long enough, i.e. throughout the lifetime of the reference of the wrapped metric. Without keeping the MutexGuard alive, we invalidate the properties of the Mutex itself, namely mutual exclusive access to the wrapped metric.

Thus far owning_ref offered a way to ship both the MutexGuard and the reference to the user, thus enforcing the lifetime bound.

Alternative approaches I can see:

  • Use parking_lot and its RwLockGuard::map method.
  • Take a FnOnce to access the wrapped metric, instead of returning a reference to the wrapped metric.
  • Clone the wrapped metric on each access.

@AbhijithGanesh
Copy link

AbhijithGanesh commented Aug 4, 2022

@mxinden I went through the solution parking_lot offers. I am unable to figure out how to deal with the MutexGuardedBuckets and that is preventing me from fixing the solution. What should I do next?

@mxinden
Copy link
Member

mxinden commented Aug 16, 2022

For the record, fix is released with v0.18.0.

#80

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants