IPC-server hardening and update #1564

Diapolo · 2012-07-06T12:03:01Z

add IMPLEMENT_RANDOMIZE_STACK for ipcThread()
log / print boost interprocess exceptions
use MAX_URI_LENGTH in guiconstants.h (also used in qrcodedialog.cpp)
remove unneeded includes and ipcShutdown() from qtipcserver.cpp
fix a small mem-leak by deleting mq before re-using it
make ipcThread() and ipcThread2() static functions
add some more comments

This is intended to improve some parts of our current IPC code, mostly security wise, it does not change the URI handling behaviour. I compiled this on Windows and made some tests with Bitcoin-Qt, everything okay.

TheBlueMatt · 2012-07-06T15:42:06Z

src/qt/qtipcserver.h

Yuck, more global flags?

After looking at how the code works, I guess I can just remove this. Even if the IPC thread doesn't run or something happened we don't rely on this to ensure a working Bitcoin-Qt. It was a left-over from my former pull.

Diapolo · 2012-07-06T16:41:29Z

Updated and removed the global and another #define, which also did not need to be there.

TheBlueMatt · 2012-07-06T16:48:24Z

src/qt/bitcoin.cpp

Now you copy the string instead of using it as-is for no clear benefit?

Well this is the same code you took a look at in the last pull? There you did not mention this. Well idea was to avoid char * where possible, as std::string should be more robust.

copying an existing const char* isnt more robust than copying its contents to a std::string.

This is what @laanwj had written to this change in the other pull:
"I think it's more consistent to use C++ string handling everywhere instead of strlen() and friends (performance is not an argument here). In most implementations, c_str doesn't perform a copy, but it simply returns a pointer to the internal buffer. Though it'd be better to use .data() if you don't rely on it being zero-terminated like in this case."

Its not necessarily more consistent to use a std::string when we already have a const char*, because you are now using both...anyway, it goes back to the "diff for diffs sake" here, there is no difference in the code (std::string(const char*) is no doubt using strlen anyway) but its just more lines of diff.

I'm going to revert this, so that in the end the diff is much smaller now (after some reverts and removing silly stuff). I consider your feedback very constructive and I'm glad you took the time to look over the code :).

Edit: Reverted

TheBlueMatt · 2012-07-06T16:55:04Z

There are some useful changes in here, by there appears to be a lot of diff here that has no purpose aside from just providing more diff?

TheBlueMatt · 2012-07-06T16:56:08Z

src/qt/guiconstants.h

Not blaming you for this, but why on earth do we have a guiconstants.h file? That just seems broken - shouldn't constants go in the header for the file in which they are used?

I think, when they are used in multiple files they should reside here, when they are ONLY used e.g. in one source-file, then yes it should just be in the header. Well this one will soon be used by qrcodedialog.cpp, too ... so at least this one should go here. I guess @laanwj introduced that file some time ago, perhaps he can explain the idea of it?

The point is if you already have to import eg qtipcserver.h in any files which are going to be using MAX_URI_LENGTH, there is no reason to have it separate in a guiconstants.h, it could just reside in qtipcserver.h, and I'd think the same is true of all the other constants here.

My point was MAX_URI_LENGTH is used in qtipcserver.cpp and qrcodedialog.cpp. Now both of them include guiconstants.h., which seems just fine, no? qtipcserver.h is of course not included in qrcodedialog.cpp.

Why is MAX_URI_LENGTH used in qrcodedialog.cpp? MAX_URI_LENGTH is not a part of the URI spec, it is part of bitcoin-qt's implementation, there is nothing wrong with a QR Code with a URI longer than MAX_URI_LENGTH.

Sometimes I feel like I'm the bad guy that did something wrong. Well in a patch some weeks ago, we introduced a limit for the URI length used in generation of QR Codes, which is the same as this one here. I indeed SEE a problem, the QR Codes which would get longer would not be valid for Bitcoin-Qt, so why not use the same limit?

See:
b1a99c3

Ah, its used for importing URIs, not exporting, sorry...I still dont like the idea of a constants header, but I suppose for MAX_URI_LENGTH it makes some sense.

I do like the constants header, as it keeps all the tweakable values together. But regard it more like a hard-coded configuration header than really "constants" as the values are arbitrary and changeable.

Diapolo · 2012-07-07T14:38:23Z

Rebased, fixing merge conflict.

TheBlueMatt · 2012-07-07T22:32:53Z

In terms of the actual code changes in this pull, ACK...the rest of it...dont care, not up to me.

laanwj · 2012-07-08T08:12:48Z

src/qt/qtipcserver.cpp

I don't get this code. Where does this 2 come from? This is not part of the diff, but it looks a bit flaky.

It was in from the very beginning, didn't ever touch it.

See: 7d145a0#L10R63 which is a commit from @TheBlueMatt

Its the same constant that is used when creating the message queue to set the maximum number of items in the queue, its essentially a fail-safe in case we are getting DoS'd.

laanwj · 2012-07-08T08:23:38Z

ACK

Diapolo · 2012-07-10T11:28:14Z

Rebased fixing a merge conflict and included some changes to the Mac exclusion code (needed after 4060d64), that needed the changed functions in it.

Diapolo · 2012-07-16T07:39:56Z

@laanwj Further problems / hints for this one?

laanwj · 2012-07-16T18:13:34Z

src/qt/qtipcserver.h

ipcThread and ipcThread2 are never called from outside qtipcserver.cpp. They should be static and local, not exported in the header file.

That is indeed a very good idea :), thanks.

ipcThread() and ipcThread2() are now static functions :).

laanwj · 2012-07-17T06:18:07Z

ACK (but needs rebase)

- add IMPLEMENT_RANDOMIZE_STACK for ipcThread() - log / print boost interprocess exceptions - use MAX_URI_LENGTH in guiconstants.h (also used in qrcodedialog.cpp) - remove unneeded includes and ipcShutdown() from qtipcserver.cpp - fix a small mem-leak by deleting mq before re-using it - make ipcThread() and ipcThread2() static functions - add some more comments

Diapolo · 2012-07-17T07:38:30Z

Rebased (was needed after the thread names patch) and removed ipcThread() and ipcThread2() from the OSX #ifdef.

Diapolo · 2012-07-17T07:40:14Z

src/qt/qtipcserver.cpp

RenameThread() is now placed here.

IPC-server hardening and update

* Change sync process: - IsBlockchainSynced(): drop CheckNodeHeight() and all complicated code, use fInitialDownload in UpdatedBlockTip() to switch initial states - ProcessTick(): detect sleep mode like it was in IsBlockchainSynced(), not by number of masternodes * Changes for sync in governance: - do not keep sync alive on ConfirmInventoryRequest() - skip some governance actions until we are synced to some level * do not run CMasternodeMan::UpdateLastPaid() until winners list is synced * start syncing mn list on the same node right after requesting sporks * replace nTimeLast<Asset> with the unified nTimeLastBumped, bump on UpdatedBlockTip * fix comments and LogPrintf-s * remove excessive MASTERNODE_SYNC_IBD * a bit more descriptive BumpAssetLastTime in few cases

TheBlueMatt reviewed Jul 6, 2012
View reviewed changes

laanwj reviewed Jul 8, 2012
View reviewed changes

laanwj reviewed Jul 16, 2012
View reviewed changes

Diapolo reviewed Jul 17, 2012
View reviewed changes

src/qt/qtipcserver.cpp

Copy link

Author

Diapolo Jul 17, 2012

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RenameThread() is now placed here.

laanwj added a commit that referenced this pull request Jul 17, 2012

Merge pull request #1564 from Diapolo/IPC-server

d47afc7

IPC-server hardening and update

laanwj merged commit d47afc7 into bitcoin:master Jul 17, 2012

nifgraup pushed a commit to nifgraup/auroracoin-project that referenced this pull request Mar 30, 2014

Merge pull request bitcoin#1564 from Diapolo/IPC-server

d00b62c

IPC-server hardening and update

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

IPC-server hardening and update #1564

IPC-server hardening and update #1564

Uh oh!

Conversation

Diapolo commented Jul 6, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Jul 6, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 6, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Jul 7, 2012

Uh oh!

TheBlueMatt commented Jul 7, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 8, 2012

Uh oh!

Diapolo commented Jul 10, 2012

Uh oh!

Diapolo commented Jul 16, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jul 17, 2012

Uh oh!

Diapolo commented Jul 17, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants