MOD-13010: Fix cmp_strings() - correctly handle binary data with embedded NULLs#7765
MOD-13010: Fix cmp_strings() - correctly handle binary data with embedded NULLs#7765
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7765 +/- ##
==========================================
- Coverage 84.77% 84.72% -0.06%
==========================================
Files 351 351
Lines 54761 54761
Branches 14727 14727
==========================================
- Hits 46426 46398 -28
- Misses 8136 8164 +28
Partials 199 199
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| static inline int cmp_strings(const char *s1, const char *s2, size_t l1, size_t l2) { | ||
| int cmp = strncmp(s1, s2, MIN(l1, l2)); | ||
| // Use memcmp instead of strncmp to correctly handle binary data with embedded NULLs | ||
| int cmp = memcmp(s1, s2, MIN(l1, l2)); |
There was a problem hiding this comment.
Seems OK - memcmp is also comparing bytes, similar to strncmp - only supporting embedded nulls.
When comparing null-terminated strings - will behave the same as before - and no extra comparisons will be done.
When comparing slices, which can contain embedded nulls, will behave as expected.
kei-nan
changed the title
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 2.10
git worktree add -d .worktree/backport-7765-to-2.10 origin/2.10
cd .worktree/backport-7765-to-2.10
git switch --create backport-7765-to-2.10
git cherry-pick -x f13ba95b1fe3d52daf9aec12ac68873d25080efe |
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin 8.4
git worktree add -d .worktree/backport-7765-to-8.4 origin/8.4
cd .worktree/backport-7765-to-8.4
git switch --create backport-7765-to-8.4
git cherry-pick -x f13ba95b1fe3d52daf9aec12ac68873d25080efe |
|
Should we aim to have some RSValue unit tests? or porting soon to Rust? |
4 participants
Describe the changes in the pull request
A clear and concise description of what the PR is solving, including:
Since
cmp_strings()usesstrcmp()the comparison ends when the first NULL character is found, and this causes wrong results whencmp_strings()is used for the comparison of byte strings.cmp_strings()is modified to usememcmp()instead ofstrcmp()Right comparison of byte strings and text.
Note
Replace strncmp with memcmp in string comparison to support embedded NULs and add a test ensuring aggregate results are coherent with binary strings.
cmp_stringsinsrc/value/value.cto usememcmp(instead ofstrncmp) for length-aware comparison supporting binary data with embedded\0.test_mod_13010intests/pytests/test_issues.pyverifying coherence betweenFT.AGGREGATEqueries with and withoutGROUPBYwhen values contain embedded NULs.Written by Cursor Bugbot for commit 89baa58. This will update automatically on new commits. Configure here.