Redis Minor comment bug

https://github.com/redis/redis/blob/90e9fc387c81d48d3d82ea7fcd2d3b6d896bf923/src/quicklist.c#L502C1-L503C82 A certain call to function quicklistReplaceEntry can make node->sz greater than 1GB

Comment From: sundb

the comment means that node->sz was already checked by if (unlikely(QL_NODE_IS_PLAIN(node) || isLargeElement(sz))).

Comment From: imchuncai

the comment means that node->sz was already checked by if (unlikely(QL_NODE_IS_PLAIN(node) || isLargeElement(sz))).

The following code will create a packed node with (node.sz >= 1GB)

TEST("pack limit")
{
        quicklist *ql = quicklistNew(-5, 0);
        quicklistPushHead(ql, "0", 1);
        quicklistPushHead(ql, "1", 1);

        /* replace with 0.5GB entry twice*/
        size_t sz = (1 << 29);
        void *s = zmalloc(sz);
        quicklistReplaceAtIndex(ql, 0, s, sz);
        quicklistReplaceAtIndex(ql, 1, s, sz);

        assert(ql->len == 1);
        quicklistNode *node = ql->head;
        assert(!QL_NODE_IS_PLAIN(node));
        assert(node->sz >= (1 << 30));

        zfree(s);
        quicklistRelease(ql);
}

Comment From: sundb

@imchuncai Yeap, you are right. In this scenario, a single node can reach a maximum of 2GB, even so, this code is still safe. Do You want to submit a PR to fix this comment?

Comment From: imchuncai

@sundb yes, I will submit.

Comment From: imchuncai

https://github.com/redis/redis/blob/90e9fc387c81d48d3d82ea7fcd2d3b6d896bf923/src/quicklist.c#L68C1-L73C31 @sundb So according to the following code, this comment is wrong either?

TEST("pack limit B")
{
        /* ql listpack is limited by count */
        quicklist *ql = quicklistNew(2, 0);
        quicklistPushHead(ql, "0", 1);
        quicklistPushHead(ql, "1", 1);

        /* replace with 0.5GB entry*/
        size_t sz = (1 << 29);
        void *s = zmalloc(sz);
        quicklistReplaceAtIndex(ql, 0, s, sz);

        assert(ql->len == 1);
        quicklistNode *node = ql->head;

        /* multi-element listpack */
        assert(!QL_NODE_IS_PLAIN(node));
        assert(node->count > 1);

        /* against SIZE_SAFETY_LIMIT */
        assert(node->sz > SIZE_SAFETY_LIMIT);

        zfree(s);
        quicklistRelease(ql);
}

TEST("pack limit C")
{
        /* ql listpack is limited by count */
        quicklist *ql = quicklistNew(2, 0);
        quicklistPushHead(ql, "0", 1);
        quicklistPushHead(ql, "1", 1);

        /* insert 0.5GB entry*/
        size_t sz = (1 << 29);
        void *s = zmalloc(sz);
        quicklistEntry entry;
        struct quicklistIter *iter =
                quicklistGetIteratorEntryAtIdx(ql, 0, &entry);
        quicklistInsertAfter(iter, &entry, s, sz);
        quicklistReleaseIterator(iter);

        assert(ql->len == 2);
        quicklistNode *node = ql->head->next;

        /* multi-element listpack */
        assert(!QL_NODE_IS_PLAIN(node));
        assert(node->count > 1);

        /* against SIZE_SAFETY_LIMIT */
        assert(node->sz > SIZE_SAFETY_LIMIT);

        zfree(s);
        quicklistRelease(ql);
}

Shall we considering about the function (quicklistReplaceEntry), (quicklistInsertBefore) and (quicklistInsertAfter) are not working as expected?

Comment From: sundb

@imchuncai i think that we might be able to modify quicklistReplaceEntry to avoid these confusions. like

    if (likely(!QL_NODE_IS_PLAIN(entry->node) &&
        (sz <= entry->sz ||
         (sz > entry->sz && _quicklistNodeAllowInsert(entry->node, quicklist->fill, sz - entry->sz)))))