[MOD-9396] Fix indexError leak by meiravgri · Pull Request #6193 · RediSearch/RediSearch

meiravgri · 2025-05-22T09:29:29Z

This PR fixes a memory leak in the IndexError object related to the handling of the global NA_rstr that is used for "N/A" responses in IndexError objects.

The issue occurred because each new IndexError object incremented the reference count of the NA_rstr via RedisModule_HoldString() but never decreased it. While not causing an immediate memory leak, this created a significant problem when the reference count reached MAX_INT - 1. At this threshold, Redis would begin treating the string as stack-allocated and start duplicating it rather than incrementing its reference count. These duplicated strings were never freed, leading to a memory leak that grew over time.

Fix

The fix is implemented in IndexError_Clear(), which now always releases the error->key.

additional changes and fixes:

IndexError_Clear called in FieldSpec_Cleanup of IndexError object held by the sepc's field was moved to IndexSpec_Free, to ensure it is always called from the main thread.
AggregatedFieldSpecInfo_Deserialize was previously intilized an IndexError object:

AggregatedFieldSpecInfo info = AggregatedFieldSpecInfo_Init(); // AggregatedFieldSpecInfo_Init calls IndexError_Init

then overriding it with new IndexError object

 info.error = IndexError_Deserialize(error, INDEX_ERROR_WITHOUT_OOM_STATUS); // IndexError_Deserialize also calls IndexError_Init

This leak was fixed by initialized info to an empty struct:

AggregatedFieldSpecInfo info = {0};

Testing

The first reference to the NA_rstr string is held by the global variable in IndexError.c.
IndexError_GlobalCleanup was introduced in this pr to release it and nullify the global variable holding it,
once a shutdown event notification is received
If the refcount is messed up, we expect the sanitizer to report a leak because the reference to the string allocation is lost.

codecov · 2025-05-22T10:01:24Z

Codecov Report

Attention: Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.

Project coverage is 88.25%. Comparing base (035a8b4) to head (4eeaa51).
Report is 10 commits behind head on master.

Files with missing lines	Patch %	Lines
src/info/index_error.c	91.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6193      +/-   ##
==========================================
- Coverage   88.26%   88.25%   -0.02%     
==========================================
  Files         223      227       +4     
  Lines       39610    39962     +352     
  Branches     2318     2592     +274     
==========================================
+ Hits        34963    35267     +304     
- Misses       4632     4680      +48     
  Partials       15       15

Flag	Coverage Δ
flow	`83.40% <94.11%> (-0.17%)`	⬇️
unit	`45.41% <88.23%> (+0.26%)`	⬆️

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.

should still leak on coordinator

src/info/index_error.c

MeirShpilraien · 2025-05-25T12:43:58Z

src/info/index_error.c

+    if (error.key != NA_rstr) {
+        RedisModule_Log(RSDummyContext, REDISMODULE_LOGLEVEL_WARNING,
+                        "NA_rstr was re-allocated");
+    }


Why here for example you chose not to assert it?

because its a valid scenario

redisearch-backport-pull-request · 2025-05-25T21:25:12Z

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-6193-to-2.8 origin/2.8
cd .worktree/backport-6193-to-2.8
git switch --create backport-6193-to-2.8
git cherry-pick -x d8b03130ef1c44a80c1f59c5a443358cc487755b

redisearch-backport-pull-request · 2025-05-25T21:25:14Z

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

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

git fetch origin 2.6
git worktree add -d .worktree/backport-6193-to-2.6 origin/2.6
cd .worktree/backport-6193-to-2.6
git switch --create backport-6193-to-2.6
git cherry-pick -x d8b03130ef1c44a80c1f59c5a443358cc487755b

redisearch-backport-pull-request · 2025-05-25T21:25:15Z

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-6193-to-2.10 origin/2.10
cd .worktree/backport-6193-to-2.10
git switch --create backport-6193-to-2.10
git cherry-pick -x d8b03130ef1c44a80c1f59c5a443358cc487755b

* free and invalidate NA_rstr (expect saniztizer to fail * fix 1 should still leak on coordinator * fix final leak - override AggregatedFieldSpecInfo and lose ref * Change the number of issues * change to 1024 * move fields IndexError_Clear to main thread * remove rm_free(spec->fields); * fix * add header --------- Co-authored-by: Dor Forer <[email protected]> (cherry picked from commit d8b0313)

redisearch-backport-pull-request · 2025-05-25T21:25:19Z

Successfully created backport PR for 8.0:

[8.0] [MOD-9396] Fix indexError leak #6206

* free and invalidate NA_rstr (expect saniztizer to fail * fix 1 should still leak on coordinator * fix final leak - override AggregatedFieldSpecInfo and lose ref * Change the number of issues * change to 1024 * move fields IndexError_Clear to main thread * remove rm_free(spec->fields); * fix * add header --------- Co-authored-by: Dor Forer <[email protected]> (cherry picked from commit d8b0313)

* [MOD-9396] Fix indexError leak (#6193) * free and invalidate NA_rstr (expect saniztizer to fail * fix 1 should still leak on coordinator * fix final leak - override AggregatedFieldSpecInfo and lose ref * Change the number of issues * change to 1024 * move fields IndexError_Clear to main thread * remove rm_free(spec->fields); * fix * add header --------- Co-authored-by: Dor Forer <[email protected]> (cherry picked from commit d8b0313) * move our of coord macro --------- Co-authored-by: Dor Forer <[email protected]>

[MOD-9396] Fix indexError leak (#6193) * free and invalidate NA_rstr (expect saniztizer to fail * fix 1 should still leak on coordinator * fix final leak - override AggregatedFieldSpecInfo and lose ref * Change the number of issues * change to 1024 * move fields IndexError_Clear to main thread * remove rm_free(spec->fields); * fix * add header --------- (cherry picked from commit d8b0313) Co-authored-by: meiravgri <[email protected]> Co-authored-by: Dor Forer <[email protected]>

* free and invalidate NA_rstr (expect saniztizer to fail * fix 1 should still leak on coordinator * fix final leak - override AggregatedFieldSpecInfo and lose ref * Change the number of issues * change to 1024 * move fields IndexError_Clear to main thread * remove rm_free(spec->fields); * fix * add header --------- Co-authored-by: Dor Forer <[email protected]> (cherry picked from commit d8b0313)

* free and invalidate NA_rstr (expect saniztizer to fail * fix 1 should still leak on coordinator * fix final leak - override AggregatedFieldSpecInfo and lose ref * Change the number of issues * change to 1024 * move fields IndexError_Clear to main thread * remove rm_free(spec->fields); * fix * add header --------- Co-authored-by: Dor Forer <[email protected]>

* [MOD-9396] Fix indexError leak (#6193) * free and invalidate NA_rstr (expect saniztizer to fail * fix 1 should still leak on coordinator * fix final leak - override AggregatedFieldSpecInfo and lose ref * Change the number of issues * change to 1024 * move fields IndexError_Clear to main thread * remove rm_free(spec->fields); * fix * add header --------- Co-authored-by: Dor Forer <[email protected]> (cherry picked from commit d8b0313) * move out of coord * move cleanup before auto memory --------- Co-authored-by: Dor Forer <[email protected]>

* [MOD-9396] Fix indexError leak (#6193) * free and invalidate NA_rstr (expect saniztizer to fail * fix 1 should still leak on coordinator * fix final leak - override AggregatedFieldSpecInfo and lose ref * Change the number of issues * change to 1024 * move fields IndexError_Clear to main thread * remove rm_free(spec->fields); * fix * add header --------- Co-authored-by: Dor Forer <[email protected]> (cherry picked from commit d8b0313) * add include * fix IndexError_GlobalCleanup * remove line --------- Co-authored-by: Dor Forer <[email protected]>

free and invalidate NA_rstr (expect saniztizer to fail

17fa1a8

github-actions bot added the size:S label May 22, 2025

meiravgri and others added 7 commits May 22, 2025 10:22

fix 1

5c541d1

should still leak on coordinator

fix final leak - override AggregatedFieldSpecInfo and lose ref

0c517a1

Change the number of issues

8720a83

change to 1024

0fd7061

move fields IndexError_Clear to main thread

da5db63

remove rm_free(spec->fields);

b210ff7

fix

5a25d36

dor-forer requested a review from MeirShpilraien May 25, 2025 12:35

dor-forer added backport 2.8 backport 2.6 backport 2.10 backport 8.0 labels May 25, 2025

dor-forer changed the title ~~free and invalidate NA_rstr (expect saniztizer to fail~~ [MOD-9396] Fix indexError leak May 25, 2025

MeirShpilraien reviewed May 25, 2025

View reviewed changes

src/info/index_error.c Show resolved Hide resolved

MeirShpilraien reviewed May 25, 2025

View reviewed changes

MeirShpilraien previously approved these changes May 25, 2025

View reviewed changes

add header

4eeaa51

dor-forer dismissed MeirShpilraien’s stale review via 4eeaa51 May 25, 2025 12:45

dor-forer requested a review from MeirShpilraien May 25, 2025 12:58

dor-forer enabled auto-merge May 25, 2025 12:58

MeirShpilraien approved these changes May 25, 2025

View reviewed changes

dor-forer added this pull request to the merge queue May 25, 2025

dor-forer removed this pull request from the merge queue due to a manual request May 25, 2025

dor-forer added this pull request to the merge queue May 25, 2025

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

meiravgri added this pull request to the merge queue May 25, 2025

meiravgri removed this pull request from the merge queue due to a manual request May 25, 2025

meiravgri added this pull request to the merge queue May 25, 2025

Merged via the queue into master with commit d8b0313 May 25, 2025
14 checks passed

meiravgri deleted the dorav_fix_indexerror_leak_simple branch May 25, 2025 21:24

redisearch-backport-pull-request bot mentioned this pull request May 25, 2025

[8.0] [MOD-9396] Fix indexError leak #6206

Merged

meiravgri mentioned this pull request May 26, 2025

[2.10] [MOD-9396] Fix indexError leak #6207

Merged

meiravgri mentioned this pull request May 26, 2025

[2.8] [MOD-9396] Fix indexError leak #6208

Merged

meiravgri mentioned this pull request May 27, 2025

[2.6] [MOD-9396] Fix indexError leak #6216

Merged

meiravgri mentioned this pull request Jun 3, 2025

[MOD-9396] Fix indexError leak #6163

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MOD-9396] Fix indexError leak #6193

[MOD-9396] Fix indexError leak #6193
meiravgri merged 9 commits intomasterfrom
dorav_fix_indexerror_leak_simple

meiravgri commented May 22, 2025 •

edited by dor-forer

Loading

Uh oh!

codecov bot commented May 22, 2025 •

edited

Loading

Uh oh!

Uh oh!

MeirShpilraien May 25, 2025

Uh oh!

dor-forer May 25, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented May 25, 2025

Uh oh!

redisearch-backport-pull-request bot commented May 25, 2025

Uh oh!

redisearch-backport-pull-request bot commented May 25, 2025

Uh oh!

redisearch-backport-pull-request bot commented May 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

meiravgri commented May 22, 2025 • edited by dor-forer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix

additional changes and fixes:

Testing

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

MeirShpilraien May 25, 2025

Choose a reason for hiding this comment

Uh oh!

dor-forer May 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

redisearch-backport-pull-request bot commented May 25, 2025

Uh oh!

redisearch-backport-pull-request bot commented May 25, 2025

Uh oh!

redisearch-backport-pull-request bot commented May 25, 2025

Uh oh!

redisearch-backport-pull-request bot commented May 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

meiravgri commented May 22, 2025 •

edited by dor-forer

Loading

codecov bot commented May 22, 2025 •

edited

Loading