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

[1.10] Possible thread safety issue in manipulation of doubly linked list of bigval_t? #54963

Open
d-netto opened this issue Jun 27, 2024 · 4 comments · Fixed by #54967
Open
Labels
GC Garbage collector

Comments

@d-netto
Copy link
Member

d-netto commented Jun 27, 2024

Up to 1.10, we had a realloc_string function in src/gc.c.

If the string we were trying to re-allocate was young and sufficiently large (> 2k bytes), realloc_string would do the following sequence of steps:

  • Unlink old_big_val from its doubly linked list of bigval_t.
  • Call realloc on old_big_val to grow the string, getting a new string new_big_val.
  • Insert new_big_val into the list of freshly allocated bigval_t.

Consider that old_big_val was allocated by thread 1, and thread 2 is the one calling realloc_string on it. At the same time, thread 1 is doing an unrelated bigval_t allocation.

Unless there is some tricky invariant that makes the doubly linked list thread-safe, seems like having thread 2 unlink one node from thread 1's list of freshly allocated bigval_t, while thread 1 inserts one node into its own list of freshly allocated bigval_t is racy.

@d-netto d-netto added the GC Garbage collector label Jun 27, 2024
@d-netto
Copy link
Member Author

d-netto commented Jun 27, 2024

CC: @kpamnany, @vtjnash, @gbaraldi.

@gbaraldi
Copy link
Member

Yeah this seems thread unsafe from a quick look.

@d-netto
Copy link
Member Author

d-netto commented Jun 27, 2024

We're experimenting with this patch (RelationalAI#160) which basically drops the realloc and makes us to allocate a new string unconditionally. @kpamnany should have the results of that in a few hours.

If it seems a reasonable fix, we're happy to upstream it.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jun 27, 2024

We could legally keep it if we first checked that it is in the correct bigobj list for the current thread, but that seems fairly slow and expensive when the logic is removed already on master

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

Successfully merging a pull request may close this issue.

3 participants