-
Notifications
You must be signed in to change notification settings - Fork 153
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
irmin-pack: some uses of Bytes.unsafe_to_string likely unsafe #1967
Comments
Good catch. I saw this related issue recently #1815 which I definitely think we ought to address before release. I think we can either create individual issues (like this one) for closing with individual PRs or close this issue to focus on making one PR that addresses all the issues we can find (and would close the earlier issue). What do you think? |
Also some comments on #1814 What I wrote then: "At the very least, all uses of Bytes.unsafe_to_string should either be obviously valid (i.e. looking locally at the lines involved can guarantee the use is correct), or there should be a comment as to why the use is valid." |
A single PR would be great, but the problem is: There will be some uses which look like they are unsafe; but if we change them, we kill performance. Clearly if something is definitely unsafe we need to change it. But given the impact on performance, we need to be careful if there is a use where it is not clear either way. So I would actually favour eg tackling all the obvious cases in a single PR, which also marks the unobvious cases with a "TODO" that we need to investigate further. |
I like that idea. Open a single PR that closes #1815 by
|
Sounds good to me. Let's hope we can mitigate any performance losses for the "obvious cases". |
An initial pass through the irmin-pack code (only), with comments on each occurrence of Bytes.unsafe_to_string: https://github.com/tomjridge/irmin/tree/investigate_unsafe_to_string We also should look at Bytes.unsafe_of_string (but these should all hopefully be "obviously safe"). |
Could we replace the local usage with |
@samoht Sorry to be dim - could you explain further? I don't follow what you mean. |
We should replace these patterns: let str =
let buf = Bytes.create n in
f buf;
Bytes.unsafe_to_string buf
in
... into: let str = with_buffer f in
... |
For a draft PR see #1970 |
Kind of critical for the release - work in on-going (@metanivek and @tomjridge are working on this) |
@metanivek has kindly let me of documenting some of the other functions (thanks!) - there is a follow up issue #1972 for the extra documentation. If people are happy I think we should merge. |
For example, in mapping_file.ml on main branch, there is:
The buffer is shared over invocations of register_entry, but register_entry calls
Bytes.unsafe_to_string
. This looks wrong to me.The text was updated successfully, but these errors were encountered: