utACK

ut ACK

ACK

utACK

There's another one here: https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L3453

Edit: and here https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L1974 although arguably, it's less important as it only logs outgoing messages.

Hmm, can't we call sanitize on the command string directly on input instead later on every LogPrint?

Somehow related: #5492

The problem here is the output, hence the point is to sanitize on output, not the input. Operations on valid input may still result in output strings that are 'dangerous'. Filtering out the characters on input would result in the counter-intuitive behavior that "get\x1bblock"is an alias for "getblock" (for another example of input and output sanitization being confused only look as far as PHP's 'magic quotes' disaster, which has corrupted texts with extra \ and didn't solve the underlying SQL injection at all).

(Prematurely rejecting commands with control or non-ASCII characters would of course be more reasonable, as there are none of those that we handle anyway)

But in any case, we'd like to minimize code impact outside of immediate logging/debugging code. E.g. @gmaxwell and I have talked about changing LogPrintStr to replace with ' ' any control characters except a '\n' at the end of the string. This would solve this problem thoroughly without any potential side-impact on the code.

Edit: and this specific code change should go in as well, it'd just be a last ditch catch-all.

@laanwj good catch; I saw the one at 1974 but because it was outgoing I didn't hit it. Since it caught your attention to, I'll just add it; better than people in the future spending time thinking about it being okay or not.