Skip to content

Conversation

@RiversJin
Copy link
Contributor

@RiversJin RiversJin commented Mar 7, 2025

The sequence LoadClusterNodes -> SetClusterNodes -> SetMasterSlaveRepl -> srv_->slot_migrator->SetStopMigrationFlag causes a null pointer access during initialization. The fix involves moving the initialization of slot_migrator earlier to resolve this issue.

PS: I'sorry that the logic where SetClusterNodes accesses slot_migrator was my modification, meaning I introduced this bug. :P

@RiversJin RiversJin force-pushed the fix/slot_migrator_nullptr branch from 53b848e to 48d4c80 Compare March 7, 2025 07:15
@RiversJin RiversJin changed the title fix(server) fix access nullptr issue in LoadClusterNodes fix: access nullptr issue in LoadClusterNodes Mar 7, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 7, 2025

@git-hulk
Copy link
Member

git-hulk commented Mar 7, 2025

@mapleFU
Copy link
Member

mapleFU commented Mar 7, 2025

By the way, anyway we should return error/emit log rather than coredump on access nullptr? Can we add some guard in visiting the pointer?

@RiversJin
Copy link
Contributor Author

LoadClusterNodes -> SetClusterNodes -> SetMasterSlaveRepl -> srv_->slot_migrator->SetStopMigrationFlag

srv_->slot_migrator->SetStopMigrationFlag(false);

@git-hulk here

@git-hulk
Copy link
Member

git-hulk commented Mar 7, 2025

LoadClusterNodes -> SetClusterNodes -> SetMasterSlaveRepl -> srv_->slot_migrator->SetStopMigrationFlag

srv_->slot_migrator->SetStopMigrationFlag(false);

@git-hulk here

@RiversJin Seems it has been checked if the migrator is null before accessing? see: https://github.com/apache/kvrocks/blob/231b093c979ab80c11ac5433e52d43213dc94840/src/cluster/cluster.cc#L250C15-L250C28

@RiversJin
Copy link
Contributor Author

LoadClusterNodes -> SetClusterNodes -> SetMasterSlaveRepl -> srv_->slot_migrator->SetStopMigrationFlag

srv_->slot_migrator->SetStopMigrationFlag(false);

@git-hulk here

Seems it has been checked if the migrator is null before accessing? see: https://github.com/apache/kvrocks/blob/231b093c979ab80c11ac5433e52d43213dc94840/src/cluster/cluster.cc#L250C15-L250C28

Oops, it seems I've mixed up the differences between the private code of my current company and the community code ..
However,according to the information above, there is another place that also relies on slot_migrator, so I'll first test locally to see if there is an issue with the current branch ...

@git-hulk
Copy link
Member

git-hulk commented Mar 7, 2025

@RiversJin Yes, but I think this PR could also be merged since it would be better to move it before loading the cluster node.

@git-hulk git-hulk merged commit a85eadd into apache:unstable Mar 7, 2025
37 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants