Page MenuHomePhabricator

[avalanche] add additional safety checks before dereferencing g_avalanche in net_processing
ClosedPublic

Authored by PiRK on May 30 2024, 12:23.

Details

Summary

Make sure every access to g_avalanche is preceded in the same scope by an early return if the pointer is null.

In ProcessMessage, we return early at the beginning of the function if IsAvalancheMessageType(msg_type) && !g_avalanche, but this is a weak guarantee as the IsAvalancheMessageType function could be buggy or not up to date.

In ProcessGetData, if the request is for a proof, we didn't actually guard against g_avalanche being a nullptr. Luckily when running bitcoind and when the node starts processing inbound messages, the processor is always already initialized. Add a continue in that loop to skip the processing if g_avalanche is nullptr anyway.

Depends on D16250

Test Plan

ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

PiRK requested review of this revision.May 30 2024, 12:23
PiRK added inline comments.
src/net_processing.cpp
5999 ↗(On Diff #48043)

g_avalanche is accessed below without a check anyway, so better rely on the above assertion.

Fabien requested changes to this revision.May 30 2024, 13:12
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/net_processing.cpp
5902 ↗(On Diff #48043)

assert is not correct here, this code is reached upon message reception so you create a potential remote triggered crash.
You can use:

if (!g_avalanche) {
    return;
}

instead.

This revision now requires changes to proceed.May 30 2024, 13:12
PiRK edited the summary of this revision. (Show Details)

use return; instead of assert in code that responds to remote messages
Remove a couple of asserts in functions that are now only called by code that verifies that the function exists.

This revision is now accepted and ready to land.May 30 2024, 20:32