Fix crash in LSET command when replacing small list items with larger ones, creating listpacks larger than 4GB #12955

sundb · 2024-01-16T12:03:57Z

The main reason for this crash is that when replacing a element of a quicklist packed node with lpReplace() method,
if the final size is larger than 4GB, lpReplace() will fail and returns NULL, causing node->entry to be incorrectly set to NULL.

Since the inserted data is not a large element, we can't just replace it like a large element, first quicklistInsertAfter()
and then quicklistDelIndex(), because the current node may be merged and invalidated in quicklistInsertAfter().

The solution of this PR:
When replacing a node fails (listpack exceeds 4GB), split the current node, create a new node to put in the middle, and try to merge them.
This is the same as inserting a large element.
In the worst case, its size will not exceed 4GB.

Release notes:
Crash in LSET command when replacing small items and exceeding 4GB

…ack limit

enjoy-binbin

did not CR, but great work, thanks for finding this

sundb · 2024-01-16T12:40:58Z

@enjoy-binbin in fact it was found by @imchuncai.

sundb · 2024-01-17T03:11:33Z

I'd actually prefer to continue ##12659, which is a better solution to the problem of over-sized nodes due to LSET commands.

oranagra

LGTM.
just to confirm, this pile of new code in quicklistReplaceEntry is reachable with other tests we have (ones not using large-memory).
i.e. if we had some off by one bug inserting the new entry in the wrong index, would it be detected?

sundb · 2024-02-06T12:38:32Z

@oranagra yes, current tests without --large-memory have 100% covered quicklistReplaceEntry(), which is verified by lcov.

…12955

Fix two crash introducted by #12955 When a quicklist node can't be inserted and split, we eventually merge the current node with its neighboring nodes after inserting, and compress the current node and its siblings. 1. When the current node is merged with another node, the current node may become invalid and can no longer be used. Solution: let `_quicklistMergeNodes()` return the merged nodes. 3. If the current node is a LZF quicklist node, its recompress will be 1. If the split node can be merged with a sibling node to become head or tail, recompress may cause the head and tail to be compressed, which is not allowed. Solution: always recompress to 0 after merging.

… 4GB (redis#12955) Fix redis#12864 The main reason for this crash is that when replacing a element of a quicklist packed node with lpReplace() method, if the final size is larger than 4GB, lpReplace() will fail and returns NULL, causing `node->entry` to be incorrectly set to NULL. Since the inserted data is not a large element, we can't just replace it like a large element, first quicklistInsertAfter() and then quicklistDelIndex(), because the current node may be merged and invalidated in quicklistInsertAfter(). The solution of this PR: When replacing a node fails (listpack exceeds 4GB), split the current node, create a new node to put in the middle, and try to merge them. This is the same as inserting a large element. In the worst case, its size will not exceed 4GB.

…edis#13040) Fix two crash introducted by redis#12955 When a quicklist node can't be inserted and split, we eventually merge the current node with its neighboring nodes after inserting, and compress the current node and its siblings. 1. When the current node is merged with another node, the current node may become invalid and can no longer be used. Solution: let `_quicklistMergeNodes()` return the merged nodes. 3. If the current node is a LZF quicklist node, its recompress will be 1. If the split node can be merged with a sibling node to become head or tail, recompress may cause the head and tail to be compressed, which is not allowed. Solution: always recompress to 0 after merging.

… 4GB (redis#12955) Fix redis#12864 The main reason for this crash is that when replacing a element of a quicklist packed node with lpReplace() method, if the final size is larger than 4GB, lpReplace() will fail and returns NULL, causing `node->entry` to be incorrectly set to NULL. Since the inserted data is not a large element, we can't just replace it like a large element, first quicklistInsertAfter() and then quicklistDelIndex(), because the current node may be merged and invalidated in quicklistInsertAfter(). The solution of this PR: When replacing a node fails (listpack exceeds 4GB), split the current node, create a new node to put in the middle, and try to merge them. This is the same as inserting a large element. In the worst case, its size will not exceed 4GB. (cherry picked from commit 1f00c95)

…edis#13040) Fix two crash introducted by redis#12955 When a quicklist node can't be inserted and split, we eventually merge the current node with its neighboring nodes after inserting, and compress the current node and its siblings. 1. When the current node is merged with another node, the current node may become invalid and can no longer be used. Solution: let `_quicklistMergeNodes()` return the merged nodes. 3. If the current node is a LZF quicklist node, its recompress will be 1. If the split node can be merged with a sibling node to become head or tail, recompress may cause the head and tail to be compressed, which is not allowed. Solution: always recompress to 0 after merging. (cherry picked from commit 1e8dc1d)

… 4GB (#12955) Fix #12864 The main reason for this crash is that when replacing a element of a quicklist packed node with lpReplace() method, if the final size is larger than 4GB, lpReplace() will fail and returns NULL, causing `node->entry` to be incorrectly set to NULL. Since the inserted data is not a large element, we can't just replace it like a large element, first quicklistInsertAfter() and then quicklistDelIndex(), because the current node may be merged and invalidated in quicklistInsertAfter(). The solution of this PR: When replacing a node fails (listpack exceeds 4GB), split the current node, create a new node to put in the middle, and try to merge them. This is the same as inserting a large element. In the worst case, its size will not exceed 4GB. (cherry picked from commit 1f00c95)

Fix two crash introducted by #12955 When a quicklist node can't be inserted and split, we eventually merge the current node with its neighboring nodes after inserting, and compress the current node and its siblings. 1. When the current node is merged with another node, the current node may become invalid and can no longer be used. Solution: let `_quicklistMergeNodes()` return the merged nodes. 3. If the current node is a LZF quicklist node, its recompress will be 1. If the split node can be merged with a sibling node to become head or tail, recompress may cause the head and tail to be compressed, which is not allowed. Solution: always recompress to 0 after merging. (cherry picked from commit 1e8dc1d)

Prevent LSET command from causing quicklist node size to exceed listp…

f2d75d2

…ack limit

sundb changed the title ~~Prevent LSET command from causing quicklist node size to exceed 4GB~~ Prevent LSET command from causing quicklist plain node size to exceed 4GB Jan 16, 2024

enjoy-binbin approved these changes Jan 16, 2024

View reviewed changes

sundb added 4 commits January 17, 2024 15:02

Unified replacing when data is large elements and nodes are full

e561ffc

Optimize code

48b03fe

Try to merge nodes after spliting

2ee75e3

Improve the test for replacing large element

b6050e1

sundb requested a review from oranagra February 6, 2024 09:05

oranagra approved these changes Feb 6, 2024

View reviewed changes

oranagra merged commit 1f00c95 into redis:unstable Feb 6, 2024
13 checks passed

oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 6, 2024

sundb added a commit to sundb/redis that referenced this pull request Feb 7, 2024

Fix crash due to use of invalid node after merge introduced by redis#…

c990d69

…12955

sundb added a commit to sundb/redis that referenced this pull request Feb 8, 2024

Fix crash due to merge of quicklist node introduced by redis#12955

585c50f

sundb added a commit to sundb/redis that referenced this pull request Feb 8, 2024

Fix crash due to merge of quicklist node introduced by redis#12955

d463645

sundb added a commit to sundb/redis that referenced this pull request Feb 8, 2024

Fix crash due to merge of quicklist node introduced by redis#12955

953eaba

sundb added a commit to sundb/redis that referenced this pull request Feb 8, 2024

Fix crash due to merge of quicklist node introduced by redis#12955

6d4e6ff

sundb mentioned this pull request Feb 8, 2024

Fix crash due to merge of quicklist node introduced by #12955 #13040

Merged

sundb deleted the lset_exceed_4gb branch March 5, 2024 03:10

oranagra changed the title ~~Prevent LSET command from causing quicklist plain node size to exceed 4GB~~ Fix crash in LSET command when replacing small list items with larger ones, creating listpacks larger than 4GB May 13, 2024

YaacovHazan mentioned this pull request May 16, 2024

Release Redis 7.2.5 #13272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash in LSET command when replacing small list items with larger ones, creating listpacks larger than 4GB #12955

Fix crash in LSET command when replacing small list items with larger ones, creating listpacks larger than 4GB #12955

sundb commented Jan 16, 2024 •

edited by oranagra

enjoy-binbin left a comment

sundb commented Jan 16, 2024

sundb commented Jan 17, 2024

oranagra left a comment

sundb commented Feb 6, 2024 •

edited

Fix crash in LSET command when replacing small list items with larger ones, creating listpacks larger than 4GB #12955

Fix crash in LSET command when replacing small list items with larger ones, creating listpacks larger than 4GB #12955

Conversation

sundb commented Jan 16, 2024 • edited by oranagra

enjoy-binbin left a comment

Choose a reason for hiding this comment

sundb commented Jan 16, 2024

sundb commented Jan 17, 2024

oranagra left a comment

Choose a reason for hiding this comment

sundb commented Feb 6, 2024 • edited

sundb commented Jan 16, 2024 •

edited by oranagra

sundb commented Feb 6, 2024 •

edited