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

net: Log to net category for exceptions in ProcessMessages #17762

Merged
merged 2 commits into from Jan 2, 2020

Conversation

@laanwj
Copy link
Member

laanwj commented Dec 17, 2019

Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.

Remove the forest of special exceptions, and simply log a short
message to the NET logging category when an exception happens during
packet processing. It is not good to panick end users with errors
that any peer can generate (let alone writing to stderr).
@laanwj laanwj added the P2P label Dec 17, 2019
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Dec 17, 2019

Tested ACK 4d88c3d

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 17, 2019

Concept ACK. I probably should have done this in #16021.

Copy link
Member

promag left a comment

ACK 4d88c3d.

} catch (...) {
PrintExceptionContinue(nullptr, "ProcessMessages()");
LogPrint(BCLog::NET, "%s(%s, %u bytes): Unknown exception caught\n", __func__, SanitizeString(strCommand), nMessageSize);

This comment has been minimized.

Copy link
@promag

promag Dec 20, 2019

Member

nit, unexpected or unhandled instead or unknown?

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 2, 2020

Author Member

It's really unknown. All known exceptions go to the other path. If it ends up here, some really weird code (maybe in a dependency?) threw an integer or bare string or such.

Copy link
Member

jnewbery left a comment

Concept ACK. One question about typeid() inline.

catch (const std::exception& e) {
PrintExceptionContinue(&e, "ProcessMessages()");
} catch (const std::exception& e) {
LogPrint(BCLog::NET, "%s(%s, %u bytes): Exception '%s' (%s) caught\n", __func__, SanitizeString(strCommand), nMessageSize, e.what(), typeid(e).name());

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 20, 2019

Member

What is typeid(e).name() for here? Here's what an example log looks like:

[msghand] ProcessMessages(tx, 85 bytes): Exception 'Superfluous witness record: iostream error' (NSt8ios_base7failureB5cxx11E) caught

Does that (NSt8ios_base7failureB5cxx11E) add any helpful information to the user?

According to https://en.cppreference.com/w/cpp/language/typeid, you need to include <typeinfo> to use typeid():

The header <typeinfo> must be included before using typeid (if the header is not included, every use of the keyword typeid makes the program ill-formed.)

This comment has been minimized.

Copy link
@hebasto

hebasto Dec 20, 2019

Member

And name():

Returns an implementation defined null-terminated character string containing the name of the type. No guarantees are given; in particular, the returned string can be identical for several types and change between invocations of the same program.

On Linux Mint 19.3 (GCC 7.4.0) with included <typeinfo> the output remains the same: something like ...(NSt8ios_base7failureB5cxx11E)....

This comment has been minimized.

Copy link
@hebasto

hebasto Dec 20, 2019

Member

Also we use the same approach here (line 544):

bitcoin/src/util/system.cpp

Lines 542 to 547 in 0cda557

if (pex)
return strprintf(
"EXCEPTION: %s \n%s \n%s in %s \n", typeid(*pex).name(), pex->what(), pszModule, pszThread);
else
return strprintf(
"UNKNOWN EXCEPTION \n%s in %s \n", pszModule, pszThread);

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 2, 2020

Author Member

This was simply borrowed from the exception logging function that we've already used since Satoshi's days. I think the idea is that it shows the type in case the exception message is not informative, I'm ok with removing it, no opinion, but I don't think it hurts…

This comment has been minimized.

Copy link
@laanwj

laanwj Jan 2, 2020

Author Member

I'll add a commit that adds #include <typeinfo> to here and system.cpp. I think that's a good point, though no C++ compiler seems to have stumbled over it.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Dec 20, 2019

ACK 4d88c3d 👃

Tested both cases:

  • Normal exception:
  • 2019-12-20T16:50:10.551972Z [msghand] received: tx (85 bytes) peer=2
  • 2019-12-20T16:50:10.552100Z [msghand] ProcessMessages(tx, 85 bytes): Exception 'Superfluous witness record: iostream error' (NSt8ios_base7failureB5cxx11E) caught
  • 2019-12-20T16:50:10.552120Z [msghand] ProcessMessages(tx, 85 bytes) FAILED peer=2
  • Something else:
  • 2019-12-20T17:03:18.898307Z [msghand] received: tx (85 bytes) peer=2
  • 2019-12-20T17:03:18.898406Z [msghand] ProcessMessages(tx, 85 bytes): Unknown exception caught
  • 2019-12-20T17:03:18.898421Z [msghand] ProcessMessages(tx, 85 bytes) FAILED peer=2
Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 4d88c3dcb61e7c075ed3dd442044e0eff4e3c8de 👃

Tested both cases:

* Normal exception:

 - 2019-12-20T16:50:10.551972Z [msghand] received: tx (85 bytes) peer=2
 - 2019-12-20T16:50:10.552100Z [msghand] ProcessMessages(tx, 85 bytes): Exception 'Superfluous witness record: iostream error' (NSt8ios_base7failureB5cxx11E) caught
 - 2019-12-20T16:50:10.552120Z [msghand] ProcessMessages(tx, 85 bytes) FAILED peer=2

* Something else:

 - 2019-12-20T17:03:18.898307Z [msghand] received: tx (85 bytes) peer=2
 - 2019-12-20T17:03:18.898406Z [msghand] ProcessMessages(tx, 85 bytes): Unknown exception caught
 - 2019-12-20T17:03:18.898421Z [msghand] ProcessMessages(tx, 85 bytes) FAILED peer=2
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjeMwwArad7Z2G99LooWjtt7/fr7X2kNavsjZh1YvI8AytS/9uZkURdVFg96geY
Q3jJqisfbeNTwJm0zsNbSWChN5OlWrZ8naGMDfyALM0oQXzj5rgCZM5WIZta4Me9
XDFq4upRfvg5QhmFI1feGX4mIYqdf2YlxYjkLz293rvTRTnQsEJ655tk1uWrm1jK
SnGKtv29IKNO0yEb4WOE4pB7muzlf3mXVO9K+21HglK5qY3Opy7hkO8NsJSpilvz
Gr4Y8yJFhJMWL8gG3fZb/YaViDzcNanbjMNXTUHIwMQZ6sQhOpDvXKYNvyNJG8Di
1xleSq1lEHp/nAdPor29fMCLVaSi7EY7A32LetW7SE30I6zbqlbH3XW4rg/mgiHr
bzoz/yJT6vrxRm8BOLYu9PjQLhKOVeDL0iGmxQaHxUdnpDdlQUYgjTeCcGUFMptB
vwD7xTEfgFdeBQuDjwFccpDDgUdUO8FBtFBHxxOEa1VVHtEvteRRvMzXCnJFaR0K
NONhQti2
=lSTE
-----END PGP SIGNATURE-----

Timestamp of file with hash df6e3a596aced982ab075caecb0b392dea1ae5573fef22c74d4cdd198ffc188c -

@hebasto

This comment has been minimized.

Copy link
Member

hebasto commented Dec 20, 2019

Concept ACK.

The use of `typeid()` for logging exception types requires this include
according to https://en.cppreference.com/w/cpp/language/typeid.
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jan 2, 2020

re-ACK 4bdd68f (only change is adding includes) 🕕

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

re-ACK 4bdd68f301a9cee3360deafc7531c638e923226b (only change is adding includes) 🕕
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUguNgwAoLtUwlt9Rl8Fd4Kkhw+iyyVzIoIfk+HL6LcG9BqfDfttfcijBiynEtNV
29HzFgff3tiBWuYeN013IqeW1rE3MYxI4pDpGqeaWKkoj9YA6PW/jPWj8okAGzAC
aYsI/2cT/H6Ea/WL7foSeMejRryRLQfD8Jg241g4Rm8iPEm0U0Tqgk5s9oqzDfMi
J1R8Et1yDZo6+enk03ytF1WW3X+SN5PkI6M4CfShnf4ZxRyQ8atYUhRMOfOb8xFA
vu2HKtYE+oWmFTfR1j6k5A9VO5ezOeL24nF8XP8FV+bcH7evzMOvudwdNMVuKag/
giW1i/ElaqHLZvTXxV1jGcu4/Vu1NfvylHQLuOft8H2BORe+OBrXAXT+qIpG5DtA
q2xyhHCfkJwItwQAMGYhauHISq93NA77CNENxyxKPvupAMfwuViYYYG+gQ8cOXg9
5YQ7GhkTFtCrTqU7abF/66XU62NKAEXvzJCjlF+/WzILoPncYAjTQcNO71pHMKYL
lM2l3l5j
=8t8Y
-----END PGP SIGNATURE-----

Timestamp of file with hash f5a70ac73ac8d01fd424479a9ace0934a6de8392add537503e92e054731e2b32 -

@promag

This comment has been minimized.

Copy link
Member

promag commented Jan 2, 2020

ACK 4bdd68f, could squash.

laanwj added a commit that referenced this pull request Jan 2, 2020
4bdd68f Add missing typeinfo includes (Wladimir J. van der Laan)
4d88c3d net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)

Pull request description:

  Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.

ACKs for top commit:
  MarcoFalke:
    re-ACK 4bdd68f (only change is adding includes) 🕕
  promag:
    ACK 4bdd68f, could squash.

Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366
@laanwj laanwj merged commit 4bdd68f into bitcoin:master Jan 2, 2020
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Jan 4, 2020
…ssMessages

4bdd68f Add missing typeinfo includes (Wladimir J. van der Laan)
4d88c3d net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)

Pull request description:

  Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.

ACKs for top commit:
  MarcoFalke:
    re-ACK 4bdd68f (only change is adding includes) 🕕
  promag:
    ACK 4bdd68f, could squash.

Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366
fanquake added a commit to fanquake/bitcoin that referenced this pull request Jan 4, 2020
Remove the forest of special exceptions, and simply log a short
message to the NET logging category when an exception happens during
packet processing. It is not good to panick end users with errors
that any peer can generate (let alone writing to stderr).

Github-Pull: bitcoin#17762
Rebased-From: 4d88c3d
fanquake added a commit to fanquake/bitcoin that referenced this pull request Jan 4, 2020
The use of `typeid()` for logging exception types requires this include
according to https://en.cppreference.com/w/cpp/language/typeid.

Github-Pull: bitcoin#17762
Rebased-From: 4bdd68f
@fanquake fanquake mentioned this pull request Jan 4, 2020
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jan 5, 2020

Being backported in 17858.

laanwj added a commit that referenced this pull request Jan 8, 2020
99b5407 scripts: fix check-symbols & check-security argument passing (fanquake)
4330a1e Update msvc build for Visual Studio 2019 v16.4 (Aaron Clauson)
b0f9b8e Moves vcpkg list to a text file and updates the appveyor job and readme to use it. (Aaron Clauson)
cd7b3b2 Updated appveyor config:  - Update build image from Visual Studio 2017 to Visual Studio 2019.  - Updated Qt static library from Qt5.9.7 to Qt5.9.8.  - Added commands to update vcpkg port files (this does not update already installed packages).  - Updated vcpkg package list as per #17309.  - Removed commands setting common project file options. Now done via common.init.vcxproj include.  - Changed msbuild verbosity from normal to quiet. Normal rights a LOT of logs and impacts appveyor job duration. Updated msvc project configs:  - Updated platform toolset from v141 to v142.  - Updated Qt static library from Qt5.9.7 to Qt5.9.8.  - Added ignore for linker warning building bitcoin-qt program.  - Added missing util/str.cpp class file to test_bitcoin project file. (Aaron Clauson)
112144d Add missing typeinfo includes (Wladimir J. van der Laan)
1a6a534 net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)
c0dc728 test: fix bitcoind already running warnings on macOS (fanquake)
5276b0e util: Add missing headers to util/fees.cpp (Hennadii Stepanov)
4d7875c rpc: require second argument only for scantxoutset start action (Andrew Chow)
bda2f5b cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice (Harris)
d14ab7c gui: disable File->CreateWallet during startup (fanquake)
b9f1bc0 wallet: unbreak with boost 1.72 (Jan Beich)

Pull request description:

  Backports the following PRs to the 0.19 branch:
  * #17654 - Unbreak build with Boost 1.72.0
  * #17695 - gui: disable File->CreateWallet during startup
  * #17687 - cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice
  * #17728 - rpc: require second argument only for scantxoutset start action
  * #17450 - util: Add missing headers to util/fees.cpp
  * #17488 - test: fix "bitcoind already running" warnings on macOS
  * #17762 - Log to net category for exceptions in ProcessMessages
  * #17364 - Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes
  * #17416 - Appveyor improvement - text file for vcpkg package list
  * #17736 - Update msvc build for Visual Studio 2019 v16.4
  * #17857 - scripts: fix symbol-check & security-check argument passing

  Fixes #17856.

ACKs for top commit:
  sipsorcery:
    ACK (tested: Windows 10 & msvc build) 99b5407.

Tree-SHA512: 91313de56fb0825e70a4be30ba0bf561b8c26d7dcf60549185df4f5e3524099398c828bb46faae807b631634d1afd5a1d397fb41e61ecfa0d746e4bf10b923cb
instagibbs added a commit to instagibbs/bitcoin that referenced this pull request Jan 22, 2020
Remove the forest of special exceptions, and simply log a short
message to the NET logging category when an exception happens during
packet processing. It is not good to panick end users with errors
that any peer can generate (let alone writing to stderr).

Github-Pull: bitcoin#17762
Rebased-From: 4d88c3d
@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Jan 22, 2020

Reviewers of this PR might want to review the somewhat related PR #17828 ("net: Use log categories when logging events that P2P peers can trigger arbitrarily").

fanquake added a commit that referenced this pull request Jan 23, 2020
…ssMessages

c89611e net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)

Pull request description:

  Backport of #17762, currently only backported to 0.19.

  This seems like something we should opportunistically plug in case wiseguys decide it's a vector to exploit to try and fill people's disks.

ACKs for top commit:
  practicalswift:
    ACK c89611e
  MarcoFalke:
    ACK c89611e, checked that this is a cherry-pick from 0.19 💐

Tree-SHA512: e48daf64a14d98a78cadd0774a597e5833a1ff19f05527dfc42f3cc38532c1c3bd1acd925c8e0c484e01fbc8c604ee2bcfc0cec0333e9af570b103a6241b657d
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Jan 23, 2020

This was backported to the 0.18 branch in #17974.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.