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

[d-netto]

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]

CC: @kpamnany, @vtjnash, @gbaraldi.

[gbaraldi]

Yeah this seems thread unsafe from a quick look.

[d-netto]

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]

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

[d-netto]

Closed by #54967.