Fix Fork GC potential double-free on error path - [MOD-12521] by GuyAv46 · Pull Request #7423 · RediSearch/RediSearch

GuyAv46 · 2025-11-19T15:26:00Z

Fix polluting out-pointers on `FGC_recvBuffer` when error occurs

Currently, if FGC_recvBuffer, it may have set the out pointer even if it fails. This makes the memory management unclear, as FGC_recvBuffer frees the buffer it allocates, but if the caller sees that the out pointer is not NULL, it may attempt to free it as well

Solution: don't set the out parameters unless the call succeeds.

This PR also improves the log printed when polling from the child fails/times out.

Mark if applicable

This PR introduces API changes
This PR introduces serialization changes

Note

Strengthens Fork GC read/recv error paths with clearer logging and safer out-params, initializes pollfd, and adds tests that simulate pipe failures during GC/apply.

Fork GC core:
- Improve FGC_recvFixed polling/error handling and add detailed log (incl. revents).
- Rework FGC_recvBuffer to use a temp length and set out-params only on success; fix potential double-free.
- Initialize gc->pollfd_read after pipe creation; minor safety cleanups (e.g., init fieldName).
Tests:
- Add testPipeErrorDuringGC and testPipeErrorDuringApply to simulate pipe closure/errors (including threaded timing) and verify graceful handling without crashes or double-frees.

^{Written by Cursor Bugbot for commit cef56a7. This will update automatically on new commits. Configure here.}

codecov · 2025-11-19T16:11:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.84%. Comparing base (7b5bd63) to head (cef56a7).
⚠️ Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7423      +/-   ##
==========================================
+ Coverage   84.82%   84.84%   +0.01%     
==========================================
  Files         346      346              
  Lines       53426    53433       +7     
  Branches    14042    14042              
==========================================
+ Hits        45321    45336      +15     
+ Misses       7909     7901       -8     
  Partials      196      196

Flag	Coverage Δ
flow	`84.86% <63.15%> (-0.10%)`	⬇️
unit	`52.18% <100.00%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/fork_gc.c

eyalrund · 2025-11-20T11:46:54Z

src/fork_gc.c

+  }
+  if (temp_len == SIZE_MAX) {
+    *len = temp_len;
    *buf = RECV_BUFFER_EMPTY;


why is that?

* make FGC_recvBuffer clean * add revents to timeout log * improve polling logs * Nullify tag field name * remove unused variable * add a test * add a stres unit-test * improve test * improve logging * add include

redisearch-backport-pull-request · 2025-11-21T06:58:37Z

Backport failed for 2.8, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.8
git worktree add -d .worktree/backport-7423-to-2.8 origin/2.8
cd .worktree/backport-7423-to-2.8
git switch --create backport-7423-to-2.8
git cherry-pick -x 442a75e07a6c285a13e938e768fdcef40ec1d5d4

redisearch-backport-pull-request · 2025-11-21T06:58:38Z

Backport failed for 2.10, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.10
git worktree add -d .worktree/backport-7423-to-2.10 origin/2.10
cd .worktree/backport-7423-to-2.10
git switch --create backport-7423-to-2.10
git cherry-pick -x 442a75e07a6c285a13e938e768fdcef40ec1d5d4

* make FGC_recvBuffer clean * add revents to timeout log * improve polling logs * Nullify tag field name * remove unused variable * add a test * add a stres unit-test * improve test * improve logging * add include (cherry picked from commit 442a75e)

redisearch-backport-pull-request · 2025-11-21T06:58:45Z

Successfully created backport PR for 8.2:

[8.2] Fix Fork GC potential double-free on error path - [MOD-12521] #7459

* make FGC_recvBuffer clean * add revents to timeout log * improve polling logs * Nullify tag field name * remove unused variable * add a test * add a stres unit-test * improve test * improve logging * add include (cherry picked from commit 442a75e)

redisearch-backport-pull-request · 2025-11-21T06:58:51Z

Successfully created backport PR for 8.4:

[8.4] Fix Fork GC potential double-free on error path - [MOD-12521] #7460

* make FGC_recvBuffer clean * add revents to timeout log * improve polling logs * Nullify tag field name * remove unused variable * add a test * add a stres unit-test * improve test * improve logging * add include (cherry picked from commit 442a75e)

…7459) Fix Fork GC potential double-free on error path - [MOD-12521] (#7423) * make FGC_recvBuffer clean * add revents to timeout log * improve polling logs * Nullify tag field name * remove unused variable * add a test * add a stres unit-test * improve test * improve logging * add include (cherry picked from commit 442a75e) Co-authored-by: GuyAv46 <[email protected]>

…7460) Fix Fork GC potential double-free on error path - [MOD-12521] (#7423) * make FGC_recvBuffer clean * add revents to timeout log * improve polling logs * Nullify tag field name * remove unused variable * add a test * add a stres unit-test * improve test * improve logging * add include (cherry picked from commit 442a75e) Co-authored-by: GuyAv46 <[email protected]>

…7461) * Fix Fork GC potential double-free on error path - [MOD-12521] (#7423) * make FGC_recvBuffer clean * add revents to timeout log * improve polling logs * Nullify tag field name * remove unused variable * add a test * add a stres unit-test * improve test * improve logging * add include (cherry picked from commit 442a75e) * reuse one thread * improvement * remove harsh stress test * fix potential double close * exit from child with RedisModule_ExitFromChild * add heavy test back

…7462) * Fix Fork GC potential double-free on error path - [MOD-12521] (#7423) * make FGC_recvBuffer clean * add revents to timeout log * improve polling logs * Nullify tag field name * remove unused variable * add a test * add a stres unit-test * improve test * improve logging * add include (cherry picked from commit 442a75e) * remove harsh stress test * fix potential double close * exit from child with RedisModule_ExitFromChild * add heavy test back

GuyAv46 added bug backport 2.8 backport 2.10 backport 8.2 backport 8.4 labels Nov 19, 2025

github-actions bot added the size:S label Nov 19, 2025

GuyAv46 requested a review from alonre24 November 19, 2025 15:43

GuyAv46 changed the title ~~Fix Fork GC potential double-free on error path - [MOD-]~~ Fix Fork GC potential double-free on error path - [MOD-12521] Nov 19, 2025

github-actions bot added the size:M label Nov 19, 2025

GuyAv46 removed the size:S label Nov 19, 2025

GuyAv46 mentioned this pull request Nov 19, 2025

Handle errors in Rust GC layer #7432

Merged

2 tasks

GuyAv46 force-pushed the guyav-fix_fork_gc_double branch from 413bed7 to 8e67ea8 Compare November 19, 2025 21:01

This comment was marked as off-topic.

Sign in to view

GuyAv46 added 10 commits November 20, 2025 11:27

make FGC_recvBuffer clean

c2950fe

add revents to timeout log

4a3bd1d

improve polling logs

ea62f44

Nullify tag field name

3dab97b

remove unused variable

d8143fe

add a test

e0ac312

add a stres unit-test

9ee5704

improve test

5e05620

improve logging

17d48af

add include

cef56a7

GuyAv46 force-pushed the guyav-fix_fork_gc_double branch from da213b1 to cef56a7 Compare November 20, 2025 09:28

GuyAv46 removed the size:M label Nov 20, 2025

github-actions bot added the size:M label Nov 20, 2025

cursor bot reviewed Nov 20, 2025

View reviewed changes

src/fork_gc.c Show resolved Hide resolved

JoanFM reviewed Nov 20, 2025

View reviewed changes

src/fork_gc.c Show resolved Hide resolved

eyalrund reviewed Nov 20, 2025

View reviewed changes

src/fork_gc.c

}

if (temp_len == SIZE_MAX) {

*len = temp_len;

*buf = RECV_BUFFER_EMPTY;

Copy link

Collaborator

eyalrund Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is that?

GuyAv46 requested review from alonre24 and eyalrund November 20, 2025 12:43

alonre24 approved these changes Nov 20, 2025

View reviewed changes

GuyAv46 added this pull request to the merge queue Nov 20, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2025

GuyAv46 added this pull request to the merge queue Nov 20, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2025

GuyAv46 added this pull request to the merge queue Nov 21, 2025

Merged via the queue into master with commit 442a75e Nov 21, 2025
25 checks passed

GuyAv46 deleted the guyav-fix_fork_gc_double branch November 21, 2025 06:58

redisearch-backport-pull-request bot mentioned this pull request Nov 21, 2025

[8.2] Fix Fork GC potential double-free on error path - [MOD-12521] #7459

Merged

redisearch-backport-pull-request bot mentioned this pull request Nov 21, 2025

[8.4] Fix Fork GC potential double-free on error path - [MOD-12521] #7460

Merged

This was referenced Nov 21, 2025

[2.10] Fix Fork GC potential double-free on error path - [MOD-12521] #7461

Merged

[2.8] Fix Fork GC potential double-free on error path - [MOD-12521] #7462

Merged

This was referenced Nov 25, 2025

[BUG] RedisSearch module crash: Redis 8.2.1 segfault in raxGenericInsert (signal 11) #7156

Closed

[CRASH] RedisSearch module crash: Redis 8.2.1 segfault in raxGenericInsert (signal 11) redis/redis#14469

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Fork GC potential double-free on error path - [MOD-12521]#7423

Fix Fork GC potential double-free on error path - [MOD-12521]#7423
GuyAv46 merged 10 commits intomasterfrom
guyav-fix_fork_gc_double

GuyAv46 commented Nov 19, 2025 •

edited by cursor bot

Loading

Uh oh!

codecov bot commented Nov 19, 2025 •

edited

Loading

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

Uh oh!

eyalrund Nov 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Nov 21, 2025

Uh oh!

redisearch-backport-pull-request bot commented Nov 21, 2025

Uh oh!

redisearch-backport-pull-request bot commented Nov 21, 2025

Uh oh!

redisearch-backport-pull-request bot commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

GuyAv46 commented Nov 19, 2025 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix polluting out-pointers on FGC_recvBuffer when error occurs

Mark if applicable

Uh oh!

codecov bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

Uh oh!

eyalrund Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented Nov 21, 2025

Uh oh!

redisearch-backport-pull-request bot commented Nov 21, 2025

Uh oh!

redisearch-backport-pull-request bot commented Nov 21, 2025

Uh oh!

redisearch-backport-pull-request bot commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GuyAv46 commented Nov 19, 2025 •

edited by cursor bot

Loading

Fix polluting out-pointers on `FGC_recvBuffer` when error occurs

codecov bot commented Nov 19, 2025 •

edited

Loading