list *listDup(list *orig)
{
    list *copy;
    listIter iter;
    listNode *node;
    int shallow_flag = 1; // first 

    if ((copy = listCreate()) == NULL)
        return NULL;
    copy->dup = orig->dup;
    copy->free = orig->free;
    copy->match = orig->match;
    listRewind(orig, &iter);
    while((node = listNext(&iter)) != NULL) {
        void *value;

        if (copy->dup) {
            value = copy->dup(node->value);
            if (value == NULL) {
                listRelease(copy);
                return NULL;
            }
           shallow_flag = 0; // second
        } else {
            value = node->value;
        }

        if (listAddNodeTail(copy, value) == NULL) {

            if (!shallow_flag && copy->free) copy->free(value); // third

            listRelease(copy);
            return NULL;
        }
    }
    return copy;
}

Comment From: sundb

I don't understand the meaning of shallow copy. Since all the OOM are handled by zmalloc_default_oom, so value will never be NULL and shallow_flag will be always 0.

Comment From: Weihnachtsmannn

I don't understand the meaning of shallow copy. Since all the OOM are handled by zmalloc_default_oom, so value will never be NULL and shallow_flag will be always 0.

If no dup function is defined in the list, a node in the orig list contains a valid element, value = node-> value means that value also references the resource. If listAddNodeTail fails (though it's unlikely to fail), and the free function is defined in the copy list, won't the data of a node in orig that has a valid element be corrupted?

Comment From: zuiderkwast

This doesn't appear to cause any problem in redis currently, but maybe it does if adlist is used separately?

We have unit tests for other internal libs (dict, listpack, sds, ...) that are enabled by the define REDIS_TEST and run from the main() function in server.c. Maybe you want to add some test for adlist to show that this is actually a bug and that your fix solves it?

Comment From: Weihnachtsmannn

This doesn't appear to cause any problem in redis currently, but maybe it does if adlist is used separately?

We have unit tests for other internal libs (dict, listpack, sds, ...) that are enabled by the define REDIS_TEST and run from the main() function in server.c. Maybe you want to add some test for adlist to show that this is actually a bug and that your fix solves

This doesn't appear to cause any problem in redis currently, but maybe it does if adlist is used separately?

We have unit tests for other internal libs (dict, listpack, sds, ...) that are enabled by the define REDIS_TEST and run from the main() function in server.c. Maybe you want to add some test for adlist to show that this is actually a bug and that your fix solves it?

I'm actually new to redis source code and treat Adlist from a general linked list perspective. Thank you for your reply.

Comment From: zuiderkwast

Ok. I agree it seems like the dup function can fail in general because the adlist should be independent, but since it is not seen in practice it's not very high prio to fix it. I think a PR with test included could be nice to have.