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

Remove unsafe #950

Open
mvdan opened this issue May 9, 2024 · 2 comments
Open

Remove unsafe #950

mvdan opened this issue May 9, 2024 · 2 comments
Labels
performance Issue related to a performance problem or pull request improving performance.

Comments

@mvdan
Copy link
Contributor

mvdan commented May 9, 2024

I count over a dozen lines which make use of https://pkg.go.dev/unsafe. This seems very odd for a TOML library. There should not be any need to do any unsafe pointer arithmetic to encode or decode TOML. Even if it makes the code a little bit easier to write or a little bit faster in some benchmarks, it also opens the possibility for all sorts of memory safety bugs, meaning that one could end up with a CVE just from decoding a bit of TOML with Go, a language that is otherwise memory safe :)

I'm happy to help remove unsafe, but I wanted to ask first. Note that the Go standard library, as well as popular third party encoding libraries like protobuf, avoid the use of unsafe as well.

@mvdan
Copy link
Contributor Author

mvdan commented May 24, 2024

Had a reply from @pelletier on Slack: basically this was for performance. He's happy with removing unsafe as long as the performance doesn't drop significantly, or perhaps if we can mostly offset it with some speedups elsewhere.

@pelletier pelletier changed the title why does this library use unsafe? Remove unsafe Aug 17, 2024
@pelletier pelletier added the performance Issue related to a performance problem or pull request improving performance. label Aug 17, 2024
@pelletier pelletier pinned this issue Aug 17, 2024
@pelletier
Copy link
Owner

Pinning this issue for visibility if anyone feels inclined to do it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Issue related to a performance problem or pull request improving performance.
Projects
None yet
Development

No branches or pull requests

2 participants