Skip to content

Fix client trackinginfo crash when tracking off by default#1684

Merged
enjoy-binbin merged 3 commits into
valkey-io:unstablefrom
yangbodong22011:fix-tracking-info-crash
Feb 10, 2025
Merged

Fix client trackinginfo crash when tracking off by default#1684
enjoy-binbin merged 3 commits into
valkey-io:unstablefrom
yangbodong22011:fix-tracking-info-crash

Conversation

@yangbodong22011

@yangbodong22011 yangbodong22011 commented Feb 7, 2025 •
edited by enjoy-binbin
Loading

Copy link
Copy Markdown
Contributor

After #1405, client trackinginfo will crash when tracking is off

/lib64/libpthread.so.0(+0xf630)[0x7fab74931630]
./src/valkey-server *:6379(clientTrackingInfoCommand+0x12b)[0x57f8db]
./src/valkey-server *:6379(call+0x5ba)[0x5a791a]
./src/valkey-server *:6379(processCommand+0x968)[0x5a8938]
./src/valkey-server *:6379(processInputBuffer+0x18d)[0x58381d]
./src/valkey-server *:6379(readQueryFromClient+0x59)[0x585ea9]
./src/valkey-server *:6379[0x46fa4d]
./src/valkey-server *:6379(aeMain+0x89)[0x5bf3e9]
./src/valkey-server *:6379(main+0x4e1)[0x455821]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fab74576555]
./src/valkey-server *:6379[0x4564f2]

The reason is that we did not init pubsub_data by default, we only
init it when tracking on.

Fixes #1683.

@yangbodong22011 yangbodong22011 force-pushed the fix-tracking-info-crash branch from 624c113 to fd7e6fd Compare February 7, 2025 10:59
@codecov

codecov Bot commented Feb 7, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.05%. Comparing base (fc55142) to head (8c2737b).
Report is 6 commits behind head on unstable.

Additional details and impacted files 
@@             Coverage Diff              @@
##           unstable    #1684      +/-   ##
============================================
+ Coverage     71.02%   71.05%   +0.03%     
============================================
  Files           121      121              
  Lines         65254    65313      +59     
============================================
+ Hits          46344    46410      +66     
+ Misses        18910    18903       -7
Files with missing lines Coverage Δ
src/networking.c 88.88% <100.00%> (+0.03%) ⬆️

... and 14 files with indirect coverage changes

@yangbodong22011 yangbodong22011 force-pushed the fix-tracking-info-crash branch from fd7e6fd to 4087b28 Compare February 7, 2025 11:15
enjoy-binbin
enjoy-binbin approved these changes Feb 7, 2025
View reviewed changes

@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.

@uriyage please take a look

zuiderkwast
zuiderkwast reviewed Feb 7, 2025
View reviewed changes
Comment thread src/networking.c Outdated
@zuiderkwast

zuiderkwast commented Feb 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Checking the flag seems to be not enough. We got this in the CI job:

Test case CLIENT TRACKINGINFO provides reasonable results when tracking on.

7635:M 07 Feb 2025 14:56:21.674 # === ASSERTION FAILED ===
7635:M 07 Feb 2025 14:56:21.674 # ==> networking.c:4104 'c->pubsub_data && c->pubsub_data->client_tracking_prefixes' is not true

@zuiderkwast zuiderkwast self-requested a review February 7, 2025 15:47
zuiderkwast
zuiderkwast reviewed Feb 7, 2025
View reviewed changes
Comment thread src/networking.c Outdated
@hpatro hpatro changed the title Fix client trackinginfo coredump when tacking off by default Fix client trackinginfo coredump when tracking off by default Feb 7, 2025
@zuiderkwast

zuiderkwast commented Feb 9, 2025

Copy link
Copy Markdown
Contributor

@yangbodong22011 Can you fix the DCO issue?

See the Details link next to the failing DCO CI job. There's some instructions about how to fix it. (git rebase HEAD~3 --signoff and force-push.)

yangbodong22011 and others added 3 commits February 10, 2025 09:39
@yangbodong22011
Fix client trackinginfo coredump when tacking off by default
9cfe76b 
This pr will fix valkey-io#1683

After valkey-io#1405, cause this
coredump.

Signed-off-by: bodong.ybd <bodong.ybd@alibaba-inc.com>
@yangbodong22011 @zuiderkwast
Update src/networking.c
de7a07e 
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: bodong.ybd <bodong.ybd@alibaba-inc.com>
@ranshid @zuiderkwast @yangbodong22011
Update src/networking.c
8c2737b 
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Signed-off-by: ranshid <88133677+ranshid@users.noreply.github.com>
Signed-off-by: bodong.ybd <bodong.ybd@alibaba-inc.com>
@yangbodong22011

yangbodong22011 commented Feb 10, 2025

Copy link
Copy Markdown
Contributor Author

@yangbodong22011 Can you fix the DCO issue?

See the Details link next to the failing DCO CI job. There's some instructions about how to fix it. (git rebase HEAD~3 --signoff and force-push.)

Sorry for late, done.

@enjoy-binbin enjoy-binbin changed the title Fix client trackinginfo coredump when tracking off by default Fix client trackinginfo crash when tracking off by default Feb 10, 2025
@enjoy-binbin enjoy-binbin merged commit 61a854d into valkey-io:unstable Feb 10, 2025
xbasel pushed a commit to xbasel/valkey that referenced this pull request Mar 27, 2025
@yangbodong22011 @enjoy-binbin
@zuiderkwast @ranshid @xbasel
Fix client trackinginfo crash when tracking off by default (valkey-io…
97a6427 
…#1684)

After valkey-io#1405, `client trackinginfo` will crash when tracking is off
```
/lib64/libpthread.so.0(+0xf630)[0x7fab74931630]
./src/valkey-server *:6379(clientTrackingInfoCommand+0x12b)[0x57f8db]
./src/valkey-server *:6379(call+0x5ba)[0x5a791a]
./src/valkey-server *:6379(processCommand+0x968)[0x5a8938]
./src/valkey-server *:6379(processInputBuffer+0x18d)[0x58381d]
./src/valkey-server *:6379(readQueryFromClient+0x59)[0x585ea9]
./src/valkey-server *:6379[0x46fa4d]
./src/valkey-server *:6379(aeMain+0x89)[0x5bf3e9]
./src/valkey-server *:6379(main+0x4e1)[0x455821]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fab74576555]
./src/valkey-server *:6379[0x4564f2]
```

The reason is that we did not init pubsub_data by default, we only
init it when tracking on.

Fixes valkey-io#1683.

Co-authored-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: ranshid <88133677+ranshid@users.noreply.github.com>
xbasel pushed a commit to xbasel/valkey that referenced this pull request Mar 27, 2025
@yangbodong22011 @enjoy-binbin
@zuiderkwast @ranshid @xbasel
Fix client trackinginfo crash when tracking off by default (valkey-io…
34e14c5 
…#1684)

After valkey-io#1405, `client trackinginfo` will crash when tracking is off
```
/lib64/libpthread.so.0(+0xf630)[0x7fab74931630]
./src/valkey-server *:6379(clientTrackingInfoCommand+0x12b)[0x57f8db]
./src/valkey-server *:6379(call+0x5ba)[0x5a791a]
./src/valkey-server *:6379(processCommand+0x968)[0x5a8938]
./src/valkey-server *:6379(processInputBuffer+0x18d)[0x58381d]
./src/valkey-server *:6379(readQueryFromClient+0x59)[0x585ea9]
./src/valkey-server *:6379[0x46fa4d]
./src/valkey-server *:6379(aeMain+0x89)[0x5bf3e9]
./src/valkey-server *:6379(main+0x4e1)[0x455821]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fab74576555]
./src/valkey-server *:6379[0x4564f2]
```

The reason is that we did not init pubsub_data by default, we only
init it when tracking on.

Fixes valkey-io#1683.

Co-authored-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: ranshid <88133677+ranshid@users.noreply.github.com>
murphyjacob4 pushed a commit to enjoy-binbin/valkey that referenced this pull request Apr 13, 2025
@yangbodong22011 @enjoy-binbin
@zuiderkwast @ranshid @murphyjacob4
Fix client trackinginfo crash when tracking off by default (valkey-io…
30d0a32 
…#1684)

After valkey-io#1405, `client trackinginfo` will crash when tracking is off
```
/lib64/libpthread.so.0(+0xf630)[0x7fab74931630]
./src/valkey-server *:6379(clientTrackingInfoCommand+0x12b)[0x57f8db]
./src/valkey-server *:6379(call+0x5ba)[0x5a791a]
./src/valkey-server *:6379(processCommand+0x968)[0x5a8938]
./src/valkey-server *:6379(processInputBuffer+0x18d)[0x58381d]
./src/valkey-server *:6379(readQueryFromClient+0x59)[0x585ea9]
./src/valkey-server *:6379[0x46fa4d]
./src/valkey-server *:6379(aeMain+0x89)[0x5bf3e9]
./src/valkey-server *:6379(main+0x4e1)[0x455821]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fab74576555]
./src/valkey-server *:6379[0x4564f2]
```

The reason is that we did not init pubsub_data by default, we only
init it when tracking on.

Fixes valkey-io#1683.

Co-authored-by: Binbin <binloveplay1314@qq.com>
Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
Co-authored-by: ranshid <88133677+ranshid@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuiderkwast zuiderkwast zuiderkwast approved these changes
@enjoy-binbin enjoy-binbin enjoy-binbin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Valkey 8.1
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CRASH] client trackinginfo coredump when tracking is off

4 participants

@yangbodong22011 @zuiderkwast @enjoy-binbin @ranshid