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

Avoid using JSON internally #139

Open
mati865 opened this issue Dec 21, 2023 · 5 comments · Fixed by #180
Open

Avoid using JSON internally #139

mati865 opened this issue Dec 21, 2023 · 5 comments · Fixed by #180
Labels
enhancement New feature or request

Comments

@mati865
Copy link
Contributor

mati865 commented Dec 21, 2023

Describe the bug you encountered:

Cargo-deny reports several issues when adding this crate to the tree. One of them is related to json crate that is unmaintained and unsound.
It'd be welcome if this crate could migrate to one of alternatives listed in the advisory.

The json crate is no longer used, but the JSON format is still unnecessarily used and should be refactored away.

What did you expect to happen instead?

Clean report from cargo-deny.

How did you install pyroscope-rs?

Added it as a dependency to one of the crates in the project.


version: pyroscope 0.5.7
os: Ubuntu 22.04

@mati865 mati865 added the bug Something isn't working label Dec 21, 2023
@korniltsev korniltsev added enhancement New feature or request and removed bug Something isn't working labels Jan 2, 2024
@korniltsev
Copy link
Collaborator

Are there any REAL problems with the library? Are there any bugs or security issues with it?

An alternative wold be to get rid of the json library alltogether.
It is currently used to pass some data withing same process from one piece of code to another, which I am not very proud of.

@korniltsev
Copy link
Collaborator

Pull requests are welcome ;)

@mati865
Copy link
Contributor Author

mati865 commented Jan 5, 2024

Are there any REAL problems with the library?

It contains undefined behaviour if as_bytes is called before attach as the report mentions: maciejhirsz/json-rust#196
Since there is no visible segfault right now I assume the struct is initialized properly but that sounds a bit fragile.
So probably the only issue right now is having to explain why it's ok to ignore this specific advisory.

https://rustsec.org/advisories/RUSTSEC-2022-0081 lists some alternatives, jzon particularly looks like drop in replacement.

An alternative wold be to get rid of the json library alltogether.
It is currently used to pass some data withing same process from one piece of code to another, which I am not very proud of.

Sounds like a viable solution as well, I'll try to look into it in the near future.

mati865 added a commit to mati865/pyroscope-rs that referenced this issue Oct 13, 2024
mati865 added a commit to mati865/pyroscope-rs that referenced this issue Oct 13, 2024
@korniltsev korniltsev reopened this Oct 14, 2024
@korniltsev
Copy link
Collaborator

I will keep the issue open as there is really no need for any json library. We should just remove it.

@mati865
Copy link
Contributor Author

mati865 commented Oct 14, 2024

Certainly, I don’t have enough time to spend on it to understand what’s happening there and do proper refactoring.

@mati865 mati865 changed the title Migrate away from json dependency that is unmaintained and unsound Avoid using JSON internally Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants