Page MenuHomePhabricator

Fix a potential segfault in the seeder
ClosedPublic

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

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rABC272aedf4f881: Fix a potential segfault in the seeder
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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Fabien created this revision.Mar 22 2019, 12:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 22 2019, 12:01
Herald added a subscriber: schancel. · View Herald Transcript
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)Mar 22 2019, 20:59
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.Wed, Mar 27, 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.Wed, Mar 27, 02:13
deadalnix accepted this revision.Wed, Mar 27, 14:37
This revision is now accepted and ready to land.Wed, Mar 27, 14:37
Fabien edited the summary of this revision. (Show Details)Wed, Mar 27, 14:47
This revision was automatically updated to reflect the committed changes.