Redis [BUG] Possible use-after-free bug on the method dictGenericDelete??

Describe the bug

Possible use-after-free bug.

To reproduce

Our static analysis tool reports it which may be a false positive.

Expected behavior

May crash the program.

Additional information

The method is here.

static dictEntry *dictGenericDelete(dict *d, const void *key, int nofree) {
    uint64_t h, idx;
    dictEntry *he, *prevHe;
    int table;

    if (d->ht[0].used == 0 && d->ht[1].used == 0) return NULL;

    if (dictIsRehashing(d)) _dictRehashStep(d);
    h = dictHashKey(d, key);

    for (table = 0; table <= 1; table++) {
        idx = h & d->ht[table].sizemask;
        he = d->ht[table].table[idx];
        prevHe = NULL;
        while(he) {
            if (key==he->key || dictCompareKeys(d, key, he->key)) {
                /* Unlink the element from the list */
                if (prevHe)
                    prevHe->next = he->next;
                else
                    d->ht[table].table[idx] = he->next;
                if (!nofree) {
                    dictFreeKey(d, he);
                    dictFreeVal(d, he);
                    zfree(he);                  //free here
                }
                d->ht[table].used--;
                return he;                     //return to caller
            }
            prevHe = he;
            he = he->next;
        }
        if (!dictIsRehashing(d)) break;
    }
    return NULL; /* not found */
}

Comment From: oranagra

@ycaibb thanks for reaching out.

it's a false positive, maybe the code should have had a comment that explains it. the return value of that function is also used an an indication if the entry was found. it is used only by these two functions:

int dictDelete(dict *ht, const void *key) {
    return dictGenericDelete(ht,key,0) ? DICT_OK : DICT_ERR;
}

dictEntry *dictUnlink(dict *ht, const void *key) {
    return dictGenericDelete(ht,key,1);
}

so the !nofree (which released that pointer) is only true in the case of dictDelete which only looks at the return value to see if it's NULL or non-NULL (doesn't use the pointer).

Comment From: ycaibb

I am very thankful for your detailed explanation. Even so, I still think it is a very dangerous operation with regard to high software quality since any careless use of the method can cause bugs in the future. Adding necessary comments is an alternative approach. Thank you.