IPC-server hardening and update #1564

Diapolo commented at 12:03 PM on July 6, 2012: none

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.

in src/qt/qtipcserver.h:None in 176324d1ef outdated

   7 | +
   8 | +// Define Bitcoin-Qt message queue maximum message number
   9 | +#define IPC_MQ_MAX_MESSAGES 2
  10 | +
  11 | +// Global state
  12 | +extern bool fIPCInitialized;

TheBlueMatt commented at 3:42 PM on July 6, 2012:

Yuck, more global flags?

Diapolo commented at 4:32 PM on July 6, 2012:

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 commented at 4:41 PM on July 6, 2012: none

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

in src/qt/bitcoin.cpp:None in 24d0e45361 outdated

 120 | @@ -121,16 +121,24 @@ int main(int argc, char *argv[])
 121 |      {
 122 |          if (boost::algorithm::istarts_with(argv[i], "bitcoin:"))
 123 |          {
 124 | -            const char *strURI = argv[i];
 125 | +            std::string strURI = argv[i];

TheBlueMatt commented at 4:48 PM on July 6, 2012:

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

Diapolo commented at 4:57 PM on July 6, 2012:

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.

TheBlueMatt commented at 4:58 PM on July 6, 2012:

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

Diapolo commented at 5:20 PM on July 6, 2012:

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."

TheBlueMatt commented at 5:46 PM on July 6, 2012:

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.

Diapolo commented at 12:32 AM on July 7, 2012:

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

in src/qt/bitcoin.cpp:None in 24d0e45361 outdated

 140 | +                // the first start of the first instance
 141 | +                if (ex.get_error_code() != boost::interprocess::not_found_error)
 142 | +                {
 143 | +                    printf("main() - boost interprocess exception #%d: %s\n", ex.get_error_code(), ex.what());
 144 | +                    break;
 145 | +                }

TheBlueMatt commented at 4:48 PM on July 6, 2012:

ACK

in src/qt/qtipcserver.cpp:None in 24d0e45361 outdated

  38 | +    catch (std::exception& e) {
  39 | +        PrintExceptionContinue(&e, "ipcThread()");
  40 | +    } catch (...) {
  41 | +        PrintExceptionContinue(NULL, "ipcThread()");
  42 | +    }
  43 | +    printf("ipcThread exited\n");

TheBlueMatt commented at 4:49 PM on July 6, 2012:

ACK

in src/qt/bitcoin.cpp:None in 24d0e45361 outdated

 274 | @@ -267,12 +275,14 @@ int main(int argc, char *argv[])
 275 |                  {
 276 |                      if (boost::algorithm::istarts_with(argv[i], "bitcoin:"))
 277 |                      {
 278 | -                        const char *strURI = argv[i];
 279 | +                        std::string strURI = argv[i];

TheBlueMatt commented at 4:51 PM on July 6, 2012:

Same as above.

Diapolo commented at 12:44 AM on July 7, 2012:

Reverted

in src/qt/qtipcserver.cpp:None in 24d0e45361 outdated

  23 | -
  24 | -using namespace boost::interprocess;
  25 | -using namespace boost::posix_time;
  26 |  using namespace boost;
  27 | -using namespace std;
  28 |

TheBlueMatt commented at 4:53 PM on July 6, 2012:

Why cant we use these namespaces?

Diapolo commented at 5:06 PM on July 6, 2012:

We can, but in terms of readability I really like to see to which namespace a function or variable belongs to. But I guess you just want me "force" in a friedly manner to revert that change ^^?

TheBlueMatt commented at 5:11 PM on July 6, 2012:

I dont have any strong opinion as to which is significantly more readable, but it seems like a ton of this pull is just diff for diffs sake, which seems overly confusing, makes it hard to verify the pull, and just seems pointless?

Diapolo commented at 5:16 PM on July 6, 2012:

I re-introduced that now, sorry ...

TheBlueMatt commented at 4:55 PM on July 6, 2012: member

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?

in src/qt/guiconstants.h:None in 24d0e45361 outdated

  24 | @@ -25,4 +25,7 @@
  25 |   */
  26 |  static const int TOOLTIP_WRAP_THRESHOLD = 80;
  27 |  
  28 | +/* Maximum allowed URI length */
  29 | +static const int MAX_URI_LENGTH = 255;
  30 | +
  31 |  #endif // GUICONSTANTS_H

TheBlueMatt commented at 4:56 PM on July 6, 2012:

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?

Diapolo commented at 4:59 PM on July 6, 2012:

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?

TheBlueMatt commented at 5:02 PM on July 6, 2012:

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.

Diapolo commented at 5:09 PM on July 6, 2012:

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.

TheBlueMatt commented at 5:12 PM on July 6, 2012:

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.

Diapolo commented at 5:23 PM on July 6, 2012:

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: https://github.com/bitcoin/bitcoin/commit/b1a99c3a1fb2613e9c7cecd565e8cc604b03eb6f

TheBlueMatt commented at 5:49 PM on July 6, 2012:

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.

laanwj commented at 11:55 AM on July 7, 2012:

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 commented at 2:38 PM on July 7, 2012: none

Rebased, fixing merge conflict.

TheBlueMatt commented at 10:32 PM on July 7, 2012: member

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

in src/qt/qtipcserver.cpp:None in b762bb7c5c outdated

  82 |      try {
  83 | -        mq = new message_queue(open_or_create, BITCOINURI_QUEUE_NAME, 2, 256);
  84 | +        mq = new message_queue(open_or_create, BITCOINURI_QUEUE_NAME, 2, MAX_URI_LENGTH);
  85 |  
  86 |          // Make sure we don't lose any bitcoin: URIs
  87 |          for (int i = 0; i < 2; i++)

laanwj commented at 8:12 AM on July 8, 2012:

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.

Diapolo commented at 10:23 AM on July 8, 2012:

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

See: https://github.com/bitcoin/bitcoin/commit/7d145a0f591dab109eae9adcfaf59303cce0431a#L10R63 which is a commit from @TheBlueMatt

TheBlueMatt commented at 10:45 PM on July 10, 2012:

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 commented at 8:23 AM on July 8, 2012: member

ACK

Diapolo commented at 11:28 AM on July 10, 2012: none

Rebased fixing a merge conflict and included some changes to the Mac exclusion code (needed after https://github.com/bitcoin/bitcoin/commit/4060d64fc9de6f11ae69f3961d4f1f0450dd8286), that needed the changed functions in it.

Diapolo commented at 7:39 AM on July 16, 2012: none

@laanwj Further problems / hints for this one?

in src/qt/qtipcserver.h:None in 2fae46f262 outdated

   0 | @@ -1,4 +1,11 @@
   1 | +#ifndef QTIPCSERVER_H
   2 | +#define QTIPCSERVER_H
   3 | +
   4 | +// Define Bitcoin-Qt message queue name
   5 |  #define BITCOINURI_QUEUE_NAME "BitcoinURI"
   6 |  
   7 | +void ipcThread(void* pArg);

laanwj commented at 6:13 PM on July 16, 2012:

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

Diapolo commented at 8:10 PM on July 16, 2012:

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

Diapolo commented at 8:37 PM on July 16, 2012:

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

in src/qt/qtipcserver.cpp:None in 1360ffffe0 outdated

  30 | +static void ipcThread2(void* pArg);
  31 |  
  32 |  #ifdef MAC_OSX
  33 |  // URI handling not implemented on OSX yet
  34 |  
  35 | +static void ipcThread(void* pArg) { }

laanwj commented at 6:17 AM on July 17, 2012:

These can go, too :) No need for dummy unused static declarations.

Diapolo commented at 7:10 AM on July 17, 2012:

So remove ipcThread() and ipcThread2() from here, but leave the OSX #ifdef and ipcInit()? Just to be sure :).

laanwj commented at 7:14 AM on July 17, 2012:

Yes, you need to retain ipcInit as it is called. The others can go :)

laanwj commented at 6:18 AM on July 17, 2012: member

ACK (but needs rebase)

IPC-server hardening and update

- 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

41c938eede

Diapolo commented at 7:38 AM on July 17, 2012: none

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

in src/qt/qtipcserver.cpp:None in 41c938eede

  43 |  {
  44 | -    message_queue::remove(BITCOINURI_QUEUE_NAME);
  45 | +    IMPLEMENT_RANDOMIZE_STACK(ipcThread(pArg));
  46 | +	
  47 | +    // Make this thread recognisable as the GUI-IPC thread
  48 | +    RenameThread("bitcoin-gui-ipc");

Diapolo commented at 7:40 AM on July 17, 2012:

RenameThread() is now placed here.

laanwj referenced this in commit d47afc7f4c on Jul 17, 2012

laanwj merged this on Jul 17, 2012

laanwj closed this on Jul 17, 2012

nifgraup referenced this in commit d00b62c2de on Mar 30, 2014

suprnurd referenced this in commit 105713c10a on Dec 5, 2017

DrahtBot locked this on Sep 8, 2021