Page MenuHomePhabricator
Differential D324

Fix segfault when node no longer connected
ClosedPublic
Actions

Authored by freetrader on Jul 13 2017, 23:07.

Details

Reviewers
deadalnix
sickpig
awemany
dagurval
CCulianu
Group Reviewers
Restricted Project
Commits
rSTAGINGb7d5bda29c07: Fix segfault when node no longer connected
rABCb7d5bda29c07: Fix segfault when node no longer connected
Summary

nodestate can be NULL when node is no longer connected,
leading to a segmentation fault (refer T64).

Added assertions to other places where node state was
obtained and later used without checking if non-null.

Test Plan

make check

Diff Detail

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

Event Timeline

freetrader created this revision.Jul 13 2017, 23:07
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJul 13 2017, 23:07
freetrader added inline comments.Jul 13 2017, 23:16
src/net_processing.cpp
450 ↗(On Diff #803)

I have omitted the check done in Core for 'nodestate->fSupportsDesiredCmpctVersion' since it's not clear to me that ABC should

// Never ask from peers who can't provide witnesses.

Open to discussion.

deadalnix requested changes to this revision.Jul 13 2017, 23:32
deadalnix added inline comments.
src/net_processing.cpp
1876 ↗(On Diff #803)

nullptr

2224 ↗(On Diff #803)

dito

2488 ↗(On Diff #803)

dito

2556 ↗(On Diff #803)

dito

This revision now requires changes to proceed.Jul 13 2017, 23:32
freetrader added inline comments.Jul 13 2017, 23:46
src/net_processing.cpp
1876 ↗(On Diff #803)

OK, in this case I'm going to move to using nullptr instead of NULL in the existing asserts in this file as well.

freetrader edited the summary of this revision. (Show Details)Jul 14 2017, 08:35
CCulianu edited edge metadata.Jul 14 2017, 08:40

Man I feel like toxic LukeJr lately. All I can come up with are critical comments. Sorry ahead of time.

You guys know that in C++, the value '0' is a sort of universally castable type to any pointer? 'NULL' is a C-ism, and has a long history. I still see plenty of C++ programmers and code using NULL, or things like 'nullptr' needlessly.

It's ok to use 0. It's perfectly good C++.

You can just

assert(nodestate)

or if you want to be verbose:

assert(nodestate != 0)

But PLEASE .. stop using NULL. It just makes me feel like I'm back in 1989 programming in C or something. C++ has 0. 0 is always null. On every architecture. No need to use "NULL". It looks.. well, strange these days. Like we are a bunch of C programmers that just learned how to use this C++ thing because it's what the kids these days are doing...

freetrader added a comment.Jul 14 2017, 08:50

Gladly moving to nullptr, I just didn't want to mix refactoring with semantic change here.

So please let's look at D325, so I can rebase this one on that one .

CCulianu added a comment.EditedJul 14 2017, 08:57

OK, aside from my bitching about the use of NULL.. the real thing is here the end result will be the same with these asserts, namely the program will EXIT (effectively crash).

We should recode all these methods to at least return gracefully with some error or something if notestate is NULL. Not exit the program!

asserts() really just are about as bad as dereferencing nodestate, obviously, when it comes to reliability of the client.

freetrader added a comment.Jul 14 2017, 09:01

No disagreement there (about the asserts not doing much for reliability of the software).

I just consider this a hotfix - we should look at the big picture and rewrite the methods doing this, but that will take much longer and users are experiencing the issue.

dagurval accepted this revision.Jul 14 2017, 09:14
dagurval added inline comments.
src/net_processing.cpp
450 ↗(On Diff #803)

fSupportsDesiredCmpctVersion can be removed from the codebase entirely since we only support one version (version == 1)

Nit: This would be easier to read if we instead just did

if (!nodestate || !nodestate->fProvidesHeaderAndIDs) {
    return;
}

so we didn't have to look for further use of nodestate after the if statement

freetrader added inline comments.Jul 14 2017, 09:21
src/net_processing.cpp
450 ↗(On Diff #803)

Thank you for the constructive feedback. That makes good sense.

freetrader updated this revision to Diff 809.Jul 14 2017, 09:59
freetrader edited edge metadata.

Add changes from review feedback

Harbormaster completed remote builds in B505: Diff 809.Jul 14 2017, 09:59
freetrader mentioned this in D325: Use nullptr instead of NULL in net_processing.cpp.Jul 14 2017, 10:00
freetrader edited the summary of this revision. (Show Details)
freetrader marked 5 inline comments as done.
freetrader added inline comments.
src/net_processing.cpp
1876 ↗(On Diff #803)

The general move to nullptr in other places in this file will be addressed in a subsequent Diff.

deadalnix added inline comments.Jul 14 2017, 10:44
src/net_processing.cpp
1876 ↗(On Diff #809)

What makes you think this must be true ?

freetrader added inline comments.Jul 14 2017, 11:06
src/net_processing.cpp
1876 ↗(On Diff #809)

If you mean why nodestate should not be null, then it's because of the dereference on line 1912:

nodestate->pindexBestHeaderSent = pindex ? pindex : chainActive.Tip();

deadalnix added inline comments.Jul 14 2017, 11:10
src/net_processing.cpp
1876 ↗(On Diff #809)

There are various control flow path in between the 2, some of them do not deref that pointer.

deadalnix requested changes to this revision.Jul 14 2017, 11:11

These asserts do not seem related to fixing the segfault. Fixing the segfault is hi pri, so there is no treason to bundle it with change on which there are still open questions.

This revision now requires changes to proceed.Jul 14 2017, 11:11
freetrader updated this revision to Diff 810.Jul 14 2017, 11:23
freetrader edited edge metadata.

Remove the asserts as requested by deadalnix

deadalnix accepted this revision.Jul 14 2017, 11:43
deadalnix added inline comments.
src/net_processing.cpp
448 ↗(On Diff #810)

On I think nodestate being null is not expected. Maybe logging something when it happen is a good idea.

This revision is now accepted and ready to land.Jul 14 2017, 11:43
freetrader updated this revision to Diff 811.Jul 14 2017, 12:00

Add a log message when nodestate is null, as requested

Harbormaster completed remote builds in B507: Diff 811.Jul 14 2017, 12:00
freetrader requested review of this revision.Jul 14 2017, 12:01
freetrader edited edge metadata.

Please have a look if you think the log message is ok.

src/net_processing.cpp
448 ↗(On Diff #810)

Added

deadalnix requested changes to this revision.Jul 14 2017, 12:15
deadalnix added inline comments.
src/net_processing.cpp
449 ↗(On Diff #811)

What the hell. `!nodestate->fProvidesHeaderAndIDs``` is not an unexpected state.

This revision now requires changes to proceed.Jul 14 2017, 12:15
freetrader updated this revision to Diff 812.Jul 14 2017, 12:20
freetrader edited edge metadata.

Fix trace

Harbormaster completed remote builds in B508: Diff 812.Jul 14 2017, 12:20
CCulianu added a comment.Jul 14 2017, 12:22

Question: when is that lNodesAnnouncingHeaderAndIDs private list cleaned up if a node should disconnect early. I don't see it being cleaned up in FinalizeNode()...

Could this be a source of bad peer id's that then don't exist in the CNoteState map?

Just a thought...

freetrader added inline comments.Jul 14 2017, 12:22
src/net_processing.cpp
449 ↗(On Diff #811)

Fixed

deadalnix accepted this revision.Jul 14 2017, 12:25
This revision is now accepted and ready to land.Jul 14 2017, 12:25
Closed by commit rABCb7d5bda29c07: Fix segfault when node no longer connected (authored by freetrader, committed by freetrader). · Explain WhyJul 14 2017, 12:26
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
src/
76 lines

Diff 813

View Options

src/net_processing.cpp

Loading...