Skip to content

Fix GEOSEARCH BYPOLYGON leak on invalid COUNT#3568

Merged
lucasyonge merged 4 commits into
valkey-io:unstablefrom
bandalgomsu:issue-3567
Apr 29, 2026
Merged

Fix GEOSEARCH BYPOLYGON leak on invalid COUNT#3568
lucasyonge merged 4 commits into
valkey-io:unstablefrom
bandalgomsu:issue-3567

Conversation

@bandalgomsu

@bandalgomsu bandalgomsu commented Apr 26, 2026

Copy link
Copy Markdown
Contributor

Free BYPOLYGON points before returning from invalid COUNT parsing paths in GEOSEARCH/GEOSEARCHSTORE.

Closes #3567

Signed-off-by: Su Ko <rhtn1128@gmail.com>
@codecov

codecov Bot commented Apr 26, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 62.50000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.40%. Comparing base (8091c6c) to head (0892e5f).
⚠️ Report is 5 commits behind head on unstable.

Files with missing lines Patch % Lines
src/geo.c 62.50% 6 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3568      +/-   ##
============================================
- Coverage     76.42%   76.40%   -0.02%     
============================================
  Files           159      159              
  Lines         80113    80128      +15     
============================================
- Hits          61225    61221       -4     
- Misses        18888    18907      +19     
Files with missing lines Coverage Δ
src/geo.c 93.91% <62.50%> (-1.30%) ⬇️

... and 19 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread tests/unit/geo.tcl Outdated
Comment thread src/geo.c
Comment thread tests/unit/geo.tcl Outdated
Signed-off-by: Su Ko <rhtn1128@gmail.com>

@enjoy-binbin enjoy-binbin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't know the details very well, however, the code structure makes it genuinely difficult to spot this issue. The most effective solution I can think of would likely be to add geoPolygonPointsFree to every error path. @KarthikSubbarao @bandalgomsu WDYT?

Also, we should perhaps standardize the usage of geoPolygonPointsFree here, rather than directly calling zfree(shape.t.polygon.points).

                /* Extract polygon vertices. */
                shape.conversion = 1;
                shape.t.polygon.num_vertices = num_vertices;
                shape.t.polygon.points = zmalloc(num_vertices * sizeof(double[2]));
                for (int j = 0; j < num_vertices * 2; j += 2) {
                    if (extractLongLatOrReply(c, c->argv + base_args + i + 2 + j, shape.t.polygon.points[j / 2]) == C_ERR) {
                        zfree(shape.t.polygon.points);
                        return;
                    }
                }
                shape.type = POLYGON_TYPE;

@bandalgomsu

Copy link
Copy Markdown
Contributor Author

I think adding geoPolygonPointsFree(&shape) to the early return paths is the safe and simple change here. We already call it in some paths where it is effectively a no-op today, such as the post-parse checks guarded by !bypolygon, so using it defensively for cleanup seems consistent with the existing pattern.

    if ((... && !bypolygon) {
        addReplyErrorFormat(c, "exactly one of FROMMEMBER or FROMLONLAT can be specified for %s",
                            (char *)objectGetVal(c->argv[0]));
        geoPolygonPointsFree(&shape);
        return;
    }

    if ((... && !(byradius || bybox || bypolygon)) {
        addReplyErrorFormat(c, "exactly one of BYRADIUS, BYBOX and BYPOLYGON can be specified for %s", (char *)objectGetVal(c->argv[0]));
        geoPolygonPointsFree(&shape);
        return;
    }

Also, we should perhaps standardize the usage of geoPolygonPointsFree here, rather than directly calling zfree(shape.t.polygon.points).

Make sense 👍

@murphyjacob4 murphyjacob4 moved this to To be backported in Valkey 8.0 Apr 29, 2026
@murphyjacob4 murphyjacob4 removed the status in Valkey 8.0 Apr 29, 2026
@murphyjacob4 murphyjacob4 moved this to To be backported in Valkey 9.0 Apr 29, 2026
@murphyjacob4 murphyjacob4 moved this to To be backported in Valkey 9.1 Apr 29, 2026
Signed-off-by: Su Ko <rhtn1128@gmail.com>

@enjoy-binbin enjoy-binbin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks!

@lucasyonge lucasyonge merged commit 7817ca8 into valkey-io:unstable Apr 29, 2026
59 checks passed
sarthakaggarwal97 pushed a commit to sarthakaggarwal97/valkey that referenced this pull request May 7, 2026
Free BYPOLYGON points before returning from invalid COUNT parsing paths
in GEOSEARCH/GEOSEARCHSTORE.

Closes valkey-io#3567

---------

Signed-off-by: Su Ko <rhtn1128@gmail.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
lucasyonge pushed a commit that referenced this pull request May 11, 2026
Free BYPOLYGON points before returning from invalid COUNT parsing paths
in GEOSEARCH/GEOSEARCHSTORE.

Closes #3567

---------

Signed-off-by: Su Ko <rhtn1128@gmail.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
lucasyonge pushed a commit that referenced this pull request May 12, 2026
Free BYPOLYGON points before returning from invalid COUNT parsing paths
in GEOSEARCH/GEOSEARCHSTORE.

Closes #3567

---------

Signed-off-by: Su Ko <rhtn1128@gmail.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
@sarthakaggarwal97 sarthakaggarwal97 moved this from To be backported to Done in Valkey 9.1 May 16, 2026
valkeyrie-ops Bot pushed a commit that referenced this pull request May 20, 2026
Free BYPOLYGON points before returning from invalid COUNT parsing paths
in GEOSEARCH/GEOSEARCHSTORE.

Closes #3567

---------

Signed-off-by: Su Ko <rhtn1128@gmail.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
valkeyrie-ops Bot pushed a commit that referenced this pull request Jun 12, 2026
Free BYPOLYGON points before returning from invalid COUNT parsing paths
in GEOSEARCH/GEOSEARCHSTORE.

Closes #3567

---------

Signed-off-by: Su Ko <rhtn1128@gmail.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: To be backported
Status: Done

Development

Successfully merging this pull request may close these issues.

[BUG] Memory leak in GEOSEARCH BYPOLYGON when followed by invalid COUNT

7 participants