Redis Potential Data Races Detected by Static Code Scanner

Hello, we are working on a race detection tool and found a potential data race in this project. The race we found does not appear to be dangerous to the core functionality of redis, but we thought it would be better to report it just in case. The race was reported on the unstable branch (commit 2ebcd63d at the time of writing).

==== Found a race between: 
line 908, column 9 in debug.c AND line 905, column 16 in debug.c
Shared variable:
 at line 72 of server.c
 72|struct redisServer server; /* Server global state */
Thread 1:
 906|        serverLogRaw(LL_WARNING|LL_RAW,
 907|        "\n\n=== REDIS BUG REPORT START: Cut & paste starting from here ===\n");
>908|        server.bug_report_start = 1;
 909|    }
 910|}
>>>Stacktrace:
>>>pthread_create [bio.c:123]
>>>  bioProcessBackgroundJobs [bio.c:123]
>>>    lazyfreeFreeObjectFromBioThread [bio.c:211]
>>>      decrRefCount [lazyfree.c:130]
>>>        freeListObject [object.c:365]
>>>          _serverPanic [object.c:291]
>>>            bugReportStart [debug.c:893]
Thread 2:
 903|
 904|void bugReportStart(void) {
>905|    if (server.bug_report_start == 0) {
 906|        serverLogRaw(LL_WARNING|LL_RAW,
 907|        "\n\n=== REDIS BUG REPORT START: Cut & paste starting from here ===\n");
>>>Stacktrace:
>>>pthread_create [networking.c:2970]
>>>  IOThreadMain [networking.c:2970]
>>>    _serverAssert [networking.c:2919]
>>>      bugReportStart [debug.c:811]

The race appears to be on the global server object declared at server.c:72.

struct redisServer server; /* Server global state */

The function bugReportStart reads server.bug_report_start at debug.c:905 and writes at debug.c:908. It looks like bugReportStart can be called by two different threads running in parallel.

void bugReportStart(void) {
    if (server.bug_report_start == 0) {  //<===== Read here
        serverLogRaw(LL_WARNING|LL_RAW,
        "\n\n=== REDIS BUG REPORT START: Cut & paste starting from here ===\n");
        server.bug_report_start = 1; //<===== Write here
    }
}

Both threads are spawned during the InitServerLast function in server.c.

void InitServerLast() {
    bioInit();        // Thread 1 spawned here
    initThreadedIO(); // Thread 2 spawned here
    set_jemalloc_bg_thread(server.jemalloc_bg_thread);
    server.initial_memory_usage = zmalloc_used_memory();
}

The first thread is spawned at bio.c:123 during the execution of bioInit. Thread 1 calls bioProcessBackgroundJobs which eventually calls bugReportStart (see full report above for stack trace).

for (j = 0; j < BIO_NUM_OPS; j++) {
        void *arg = (void*)(unsigned long) j;
        if (pthread_create(&thread,&attr,bioProcessBackgroundJobs,arg) != 0) {
            serverLog(LL_WARNING,"Fatal: Can't initialize Background Jobs.");
            exit(1);
        }
        bio_threads[j] = thread;
    }

The second thread is spawned at networking.c:2970 during the execution of initThreadedIO. Thread 2 calls IOThreadMain which also eventually calls bugReportStart (see full report above for stack trace).

/* Spawn and initialize the I/O threads. */
    for (int i = 0; i < server.io_threads_num; i++) {
        // ...
        pthread_mutex_lock(&io_threads_mutex[i]); /* Thread will be stopped. */
        if (pthread_create(&tid,NULL,IOThreadMain,(void*)(long)i) != 0) {
            serverLog(LL_WARNING,"Fatal: Can't initialize IO thread.");
            exit(1);
        }
        io_threads[i] = tid;
    }

There does not appear to be any synchronizations that would prevent each thread from executing bugReportStart in parallel, leading to a read/write race on server.bug_report_start.

Comment From: ShooterIT

Hi @BradSwain I need a tool to detect data race of redis, would you mind telling me the name of the tool you used

Comment From: BradSwain

Hi @ShooterIT I used a new tool we are developing called Coderrect Scanner. You can download the tool here: https://coderrect.com/download/ If you have any questions or comments feel free to reach out to me at brad@coderrect.com

Comment From: ShooterIT

Copy that, thanks @BradSwain