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

fix a concurrency memory-safety bug in Buffer #12104

Merged
merged 2 commits into from Mar 13, 2023

Conversation

gasche
Copy link
Member

@gasche gasche commented Mar 13, 2023

Unlike all other add_* functions, Buffer.add_string was reading b.position twice within a code section performing unsafe operations; the unsafe operations could break memory safety in case of race on b.position between the two reads.

closes #12103

@gasche
Copy link
Member Author

gasche commented Mar 13, 2023

Note: we probably want a Changes entry for this minor fix, because it is an observable bug in a released version. On the other hand, it is pretty strange that there are no Change entries for #11279 and #11742 which fixed all the other issues and required substantially more work. I will add a change entry for those, in the current PR (ugh!).

Unlike all other add_* functions, Buffer.add_string was reading
b.position twice within a code section performing unsafe operations;
the unsafe operations could break memory safety in case of race on
b.position between the two reads.
@gasche gasche force-pushed the fix-buffer-concurrency-bug branch from 1ff92d2 to 4be7565 Compare March 13, 2023 11:59
Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously correct fix, thanks !
And rereading the code, this was the only place where we had two reads on the same location in the unsafe path.

@gasche gasche merged commit 584d6ff into ocaml:trunk Mar 13, 2023
9 checks passed
@gasche
Copy link
Member Author

gasche commented Mar 13, 2023

Ah, I guess I forgot to update the Changes entry with Florian as review, I will hot-fix right now.

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 this pull request may close these issues.

Assertion failure with Buffer in the bytecode debug runtime
2 participants