Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

hinted handoff: Possible dereferencing a null pointer #21699

Open
dawmd opened this issue Nov 26, 2024 · 0 comments · May be fixed by #21676
Open

hinted handoff: Possible dereferencing a null pointer #21699

dawmd opened this issue Nov 26, 2024 · 0 comments · May be fixed by #21676
Assignees
Labels
area/hinted handoff Issues related to Hinted Handoff backport/6.1 should be backported to 6.1 backport/6.2 should be backported to 6.2 bug
Milestone

Comments

@dawmd
Copy link
Contributor

dawmd commented Nov 26, 2024

In this piece of code:

/// Checks if there is a node corresponding to a given host ID that hasn't been down for longer
/// than a given amount of time. The function relies on information obtained from the passed `gms::gossiper`.
static bool endpoint_downtime_not_bigger_than(const gms::gossiper& gossiper, const locator::host_id& host_id,
uint64_t max_downtime_us)
{
// We want to enforce small buffer optimization in the call
// to `gms::gossiper::for_each_endpoint_state_until()` below
// to avoid an unnecessary allocation.
// Since we need all these four pieces of information in the lambda,
// the function object passed to the function might be too big.
// That's why we create it locally on the stack and only pass a reference to it.
struct sbo_info {
locator::host_id host_id;
const gms::gossiper& gossiper;
int64_t max_hint_window_us;
bool small_node_downtime;
};
sbo_info info {
.host_id = host_id,
.gossiper = gossiper,
.max_hint_window_us = max_downtime_us,
.small_node_downtime = false
};
gossiper.for_each_endpoint_state_until(
[&info] (const gms::inet_address& ip, const gms::endpoint_state& state) {
const auto app_state = state.get_application_state_ptr(gms::application_state::HOST_ID);
const auto host_id = locator::host_id{utils::UUID{app_state->value()}};
if (!app_state || host_id != info.host_id) {
return stop_iteration::no;
}
if (info.gossiper.get_endpoint_downtime(ip) <= info.max_hint_window_us) {
info.small_node_downtime = true;
return stop_iteration::yes;
}
return stop_iteration::no;
});
return info.small_node_downtime;
}

we don't check whether app_state is not a null pointer before dereferencing it. It should not be a null pointer in normal circumstances, but as #21666 showed, it sometimes can. We should follow the good programming practices and fix it.

@dawmd dawmd added area/hinted handoff Issues related to Hinted Handoff backport/6.1 should be backported to 6.1 backport/6.2 should be backported to 6.2 bug labels Nov 26, 2024
@dawmd dawmd added this to the 6.3 milestone Nov 26, 2024
@dawmd dawmd self-assigned this Nov 26, 2024
dawmd added a commit to dawmd/scylladb that referenced this issue Nov 26, 2024
Before these changes, we dereferenced `app_state` in
`manager::endpoint_downtime_not_bigger_than()` before checking that it's
not a null pointer. We fix that.

Fixes scylladb#21699
@dawmd dawmd linked a pull request Nov 26, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hinted handoff Issues related to Hinted Handoff backport/6.1 should be backported to 6.1 backport/6.2 should be backported to 6.2 bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant