[BUG] NULL Pointer Dereference on rax.c/redis-cli.c using static analysis

[geraldo-netto]

Dear Friends,

while playing with static analysis (clang static analyser) on Redis 6.2.1, I found a few null pointer dereferences

on rax.c, there is this block:

oom:
    /* This code path handles out of memory after part of the sub-tree was
     * already modified. Set the node as a key, and then remove it. However we
     * do that only if the node is a terminal node, otherwise if the OOM
     * happened reallocating a node in the middle, we don't need to free
     * anything. */
    if (h->size == 0) { // Access to field 'size' results in a dereference of a null pointer (loaded from variable 'h')
        h->isnull = 1;
        h->iskey = 1;
        rax->numele++; /* Compensate the next remove. */
        assert(raxRemove(rax,s,i,NULL) != 0);
    }
    errno = ENOMEM;
    return 0;
}

Where we can avoid a null pointer dereference that can happen under rare conditions by applying the following change:

if (h != NULL && h->size == 0) {
    ...
}

on redis-cli.c, there are 2 possible null pointer dereferences: The first one is this:

while (k < HOTKEYS_SAMPLE && freqs[i] > counters[k]) k++; // Access to field 'freqs' can result in a dereference of a null pointer

Where we can avoid a null pointer dereference that can happen under rare conditions by applying the following change:

while (k < HOTKEYS_SAMPLE && (freqs != NULL && freqs[i] > counters[k])) k++;

The second one is this:

if(!types[i] || (!types[i]->sizecmd && !memkeys)) // Access to field 'types' can result in a dereference of a null pointer

Where we can avoid a null pointer dereference that can happen under rare conditions by applying the following change:

if(types != NULL && (!types[i] || (!types[i]->sizecmd && !memkeys)))

Would you mind considering applying these changes?

Thank You so much, Geraldo Netto

[yossigo]

@geraldo-netto Did you manually analyze the code paths that the static analyzer reported? Had a quick look and they seem like false positives to me.

[geraldo-netto]

Hi @yossigo ,

I have uploaded the full scan here: https://exdev.sourceforge.io/redis-6.2.1/ and I agree with you that at first sight they are false-positive but considering C limitations, I think it's better to be safe than sorry Also, some of those very rare conditions require as much as 115 steps to happen...

I hope this can be somehow useful

Cheers, Geraldo Netto

[yossigo]

Thank you @geraldo-netto, I agree that C always calls for extra safety so additional manual analysis and judgement are always required.

I don't have the bandwidth for this at the moment but if anyone wants to pursue the analysis of this report, some real problems can be found and fixed.

[geraldo-netto]

Hi @yossigo !

Sure, I understand! :) I'll double-check and propose some patches later

Keep Rocking, Geraldo Ntto

[OhBonsai]

Hi @geraldo-netto:

I also want to participate some case in your report. Maybe you could add some checkbox? 😸

[geraldo-netto]

Hi @OhBonsai ,

Sure, please, check https://clang-analyzer.llvm.org/ It might take a while for me to propose a fix for each thing, but let's keep it up!!! :)