Redis [BUG]Possible NULL pointer dereference on eventLoop->timeEventNextId++??

Describe the bug Hi, our static tool reports a null pointer dereference on this site. The reason is aeCreateEventLoop go to error state and return null to config.el, so eventLoop->timeEventNextId should be checked before dereference. It may be a false positive. Thank you for your confirmation.

aeEventLoop *aeCreateEventLoop(int setsize) {
    aeEventLoop *eventLoop;
    int i;

    monotonicInit();    /* just in case the calling app didn't initialize */

    if ((eventLoop = zmalloc(sizeof(*eventLoop))) == NULL) goto err;
    eventLoop->events = zmalloc(sizeof(aeFileEvent)*setsize);
    eventLoop->fired = zmalloc(sizeof(aeFiredEvent)*setsize);
    if (eventLoop->events == NULL || eventLoop->fired == NULL) goto err;
    eventLoop->setsize = setsize;
    eventLoop->timeEventHead = NULL;
    eventLoop->timeEventNextId = 0;
    eventLoop->stop = 0;
    eventLoop->maxfd = -1;
    eventLoop->beforesleep = NULL;
    eventLoop->aftersleep = NULL;
    eventLoop->flags = 0;
    if (aeApiCreate(eventLoop) == -1) goto err;
    /* Events with mask == AE_NONE are not set. So let's initialize the
     * vector with it. */
    for (i = 0; i < setsize; i++)
        eventLoop->events[i].mask = AE_NONE;
    return eventLoop;

err:
    if (eventLoop) {
        zfree(eventLoop->events);
        zfree(eventLoop->fired);
        zfree(eventLoop);
    }
    return NULL;
}

int main(int argc, const char **argv) {
   ...;
    config.el = aeCreateEventLoop(1024*10); // config.el  may be null
    aeCreateTimeEvent(config.el,1,showThroughput,NULL,NULL);
   ...;
}

long long aeCreateTimeEvent(aeEventLoop *eventLoop, long long milliseconds,
        aeTimeProc *proc, void *clientData,
        aeEventFinalizerProc *finalizerProc)
{
    long long id = eventLoop->timeEventNextId++; //should check non-null before dereference
    ...;
    return id;
}

To reproduce

Steps to reproduce the behavior and/or a minimal code sample.

Expected behavior

A description of what you expected to happen.

Additional information

Any additional information that is relevant to the problem.

Comment From: ycaibb

A similar reason can lead to NULL pointer dereference on the below function.

static benchmarkThread *createBenchmarkThread(int index) {
    benchmarkThread *thread = zmalloc(sizeof(*thread));
    if (thread == NULL) return NULL;
    thread->index = index;
    thread->el = aeCreateEventLoop(1024*10); //return null 
    aeCreateTimeEvent(thread->el,1,showThroughput,NULL,NULL); //dereference on this method.
    return thread;
}

Comment From: oranagra

@ycaibb you're referring to a case where zmalloc returns NULL, but this will never happen in Redis. See zmalloc_oom_handler So redis doesn't need to handle failures to allocate memory.

Comment From: ycaibb

OK. Thank you for your explanation.

Comment From: ycaibb

Because this method never go to the err state, why do the developers still write this part?

err:
    if (eventLoop) {
        zfree(eventLoop->events);
        zfree(eventLoop->fired);
        zfree(eventLoop);
    }
    return NULL;

Comment From: oranagra

Some cases it's out of habit, in other cases to write clean code. Or to handle other errors.

In the specific case you're referring to, the err label is reachable in other errors, so these still need to clean after them and not leak. But the goto that checks the NULL return from zmalloc, is dead code.

Btw, also note that redis doesn't attempt to release memory before exiting (would be a waste of time and will just delay the shutdown or restart). But some pieces of code are written as a generic unit, and they should be doing good cleanup code since they can't assume the caller will always exit if they return an error.

Comment From: ycaibb

OK. Thank you for your explanation.