vmware / splinterdb

High Performance Embedded Key-Value Store
https://splinterdb.org
Apache License 2.0
685 stars 57 forks source link

Potential use of uninitialized variable, thread_tid, in task_register_thread() in assert code. #499

Closed gapisback closed 1 year ago

gapisback commented 1 year ago

During the review of PR #469 it was noted that we are probably referencing an uninitialized variable in this code block at the top of task_register_thread():

186    /*
187     * For all newly created threads, the thread-index will be 0 before the hook
188     * function, run below, generates this thread's index. We do not want to
189     * register a thread twice; otherwise, it will simply burn up an index and
190     * possibly also waste allocated scratch space.
191     * As threads generally will require scratch space to be allocated, an
192     * attempt to re-register a thread should find scratch space already
193     * allocated. Flag that.
194     */
195    platform_assert((thread_tid == 0)
196                       || (ts->thread_scratch[thread_tid] == NULL),
197                    "[%s:%d::%s()] Thread at index %lu is found to have scratch"
198                    " space already allocated, %p\n",

This assert was added previously under SHA bda1e46 as part of PR #448 .

platform_get_tid() returns extern __thread threadid xxxtid declared in platform_linux/platform.c: 10 __thread threadid xxxtid;

While implementing the fix for PR #448, the thought was that this thread-local variable would be initialized to 0 but documentation suggests that this is not guaranteed.

It seems that we just got lucky that this assert on L195 is not tripping more often. Need to make this check more reliable and robust.

--- Updated 30.Dec.2022: Although slightly unrelated, plan to do this cleanup after the rework of fg-/bg-threads that is in-flight under PR #497 hits /main. That may affect some of this area, so want to wait till those changes are finalized.