Page MenuHomePhabricator

Fix a potential segfault in the seeder
ClosedPublic

Authored by Fabien on Mar 22 2019, 12:01.

Details

Summary

The seeded software is doing statistics on tested nodes before any node get tested.
This diff addresses the issue by ensuring that the list of tested node is not empty before statistics are built on top.

Implementation details:
This bug has been revealed by running the seeder on OSX.
The CAddrDb::GetStats() method can access internal dequeue and map
members by index despite their size is zero, causing a segfault.

This diff fixes it by:

  • Testing the size of the list of tested nodes to be > 0 before accessing, ensuring data exists before building the stats.
  • Using at() for accessing instead of []. This function checks for out-of-bounds accesses and will throw an exception instead of a segfault, making the debugging much easier. It also avoids adding inadvertently an entry in the map (this is also enforced by making the method const).
Test Plan

Build with various compilers and run the seeder on various platforms.
Currently tested on Linux and OSX (the seeder is unsupported on
Windows).
Note: not all combinations would give a segfault before this patch.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
fix_bitcoin_seeder_segfault
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5271
Build 8605: Bitcoin ABC Buildbot (legacy)
Build 8604: arc lint + arc unit

Event Timeline

deadalnix requested changes to this revision.Mar 22 2019, 13:39

I doesn't looks like you know what the problem actually is, and therefore you have no way to know if the solution is appropriate (and I do not either).

This revision now requires changes to proceed.Mar 22 2019, 13:39
Fabien edited the summary of this revision. (Show Details)
Fabien requested review of this revision.Mar 22 2019, 21:02

@deadalnix I updated the summary, please let me know if this shows the problem more clearly.

deadalnix requested changes to this revision.Mar 27 2019, 02:13

If ourId is accessed concurrently by several threads, then this doesn't fix the problem. If it isn't then what is this fixing? I don't see any description of the problem that lead me to think this solution actually fixes anything.

This revision now requires changes to proceed.Mar 27 2019, 02:13
This revision is now accepted and ready to land.Mar 27 2019, 14:37
This revision was automatically updated to reflect the committed changes.