Net: Improvements to Tor control port parser #10408

pull str4d wants to merge 6 commits into bitcoin:master from str4d:torcontrol-parser-patches changing 3 files +275 −7

str4d commented at 6:23 AM on May 16, 2017: contributor
Cherry-picked from the following Zcash PRs:
- https://github.com/zcash/zcash/pull/2177
- https://github.com/zcash/zcash/pull/2251
torcontrol: Improve comments d8e03c0340
torcontrol: Add unit tests for Tor reply parsers 29f3c20078

torcontrol: Fix ParseTorReplyMapping

- Ignore remaining input if it is an OptArguments
- Correctly handle escapes

d63677bbb2

torcontrol: Check for reading errors in ReadBinaryFile

This ensures that ReadBinaryFile never returns exactly TOR_COOKIE_SIZE bytes if
the file was larger than that.

0b6f40d4ca

torcontrol: Log invalid parameters in Tor reply strings where meaningful 0182a11737

torcontrol: Handle escapes in Tor QuotedStrings

https://trac.torproject.org/projects/tor/ticket/14999 is tracking an encoding
bug with the Tor control protocol, where many of the QuotedString instances that
Tor outputs are in fact CStrings, but it is not documented which ones are which.

https://spec.torproject.org/control-spec section 2.1.1 provides a future-proofed
rule for handing QuotedStrings, which this commit implements.

This commit merges all six commits from https://github.com/zcash/zcash/pull/2251

49a199bb51

jonasschnelli added the label P2P on May 16, 2017
in src/torcontrol.cpp:382 in 49a199bb51
```
 375 | @@ -323,6 +376,10 @@ static std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size
 376 |      char buffer[128];
 377 |      size_t n;
 378 |      while ((n=fread(buffer, 1, sizeof(buffer), f)) > 0) {
 379 | +        // Check for reading errors so we don't return any data if we couldn't
 380 | +        // read the entire file (or up to maxsize)
 381 | +        if (ferror(f))
 382 | +            return std::make_pair(false,"");
```
laanwj commented at 8:09 AM on May 17, 2017:

Wouldn't the fread already return <=0 in the case of an error? (making it necessary to move the check outside of the loop)

str4d commented at 12:06 PM on May 17, 2017:

No, there is no such guarantee:

If [the return value] differs from the count parameter, either a reading error occurred or the end-of-file was reached while reading. In both cases, the proper indicator is set, which can be checked with ferror and feof, respectively.

So it is entirely possible for fread to return a number 0 < retval < count if a reading error occurs.

str4d commented at 12:22 PM on May 17, 2017:

The reason we added this check was the possibility that a read error could occur at byte 33, resulting in 32 bytes of a larger file being returned and used as the cookie (which checks for exact length), in violation of control-spec. Thus we only need to defend against the case where fread returns a value greater than zero on error; since the buffer is larger than the cookie size, the only remaining way to return a 32-byte read is by reaching EOF.

laanwj commented at 5:29 PM on May 17, 2017:

Interesting, I didn't know that, I though that only the EOF (or interrupted by signal) case would cause it to return a smaller amount larger or equal to zero (which according to the manual page is the case for read, but apparently not fread).
laanwj commented at 8:14 AM on May 17, 2017: member

Thanks for the improvements; I still have to review the unescaping loop in detail but overall it looks good to me.
laanwj commented at 5:58 PM on May 18, 2017: member

utACK 49a199b
laanwj merged this on May 18, 2017
laanwj closed this on May 18, 2017
laanwj referenced this in commit 28c6e8d71b on May 18, 2017
practicalswift referenced this in commit f2fb132cb0 on Jun 14, 2017
str4d deleted the branch on Jun 23, 2017
zkbot referenced this in commit 90a255a747 on Jul 10, 2017
HashUnlimited referenced this in commit 11610435c4 on Mar 6, 2018
MarcoFalke referenced this in commit 536120ec39 on May 24, 2018
PastaPastaPasta referenced this in commit d943d03a88 on Jun 10, 2019
PastaPastaPasta referenced this in commit 53bffb89cf on Jun 11, 2019
PastaPastaPasta referenced this in commit ba07e1e5b4 on Jun 11, 2019
PastaPastaPasta referenced this in commit d0b77dbb4f on Jun 15, 2019
PastaPastaPasta referenced this in commit ba0879e73f on Jun 19, 2019
PastaPastaPasta referenced this in commit 94c422e17b on Jun 19, 2019
PastaPastaPasta referenced this in commit 3367659264 on Jun 19, 2019
PastaPastaPasta referenced this in commit 4a213c6841 on Jun 19, 2019
PastaPastaPasta referenced this in commit 8fbfa85577 on Jun 19, 2019
PastaPastaPasta referenced this in commit a3b04b6a99 on Jun 20, 2019
PastaPastaPasta referenced this in commit b9b912e199 on Jun 22, 2019
PastaPastaPasta referenced this in commit 92a986d45a on Jun 22, 2019
PastaPastaPasta referenced this in commit 173b5134c5 on Jun 22, 2019
PastaPastaPasta referenced this in commit a9b28922e1 on Jun 22, 2019
PastaPastaPasta referenced this in commit f1ee9af3d4 on Jun 22, 2019
PastaPastaPasta referenced this in commit 7ebe41b4b4 on Jun 24, 2019
lateminer referenced this in commit 36835a363a on Jun 28, 2019
barrystyle referenced this in commit 23234b1e1a on Jan 22, 2020
furszy referenced this in commit a9c922b96b on Apr 2, 2020
random-zebra referenced this in commit 0a4072e4fc on Apr 4, 2020
PastaPastaPasta referenced this in commit 92aeba497d on Jun 17, 2020
PastaPastaPasta referenced this in commit baede72c4e on Jun 27, 2020
PastaPastaPasta referenced this in commit eecc792fb9 on Jun 28, 2020
PastaPastaPasta referenced this in commit 443ceee1e5 on Jun 29, 2020
PastaPastaPasta referenced this in commit 9b8f67eb04 on Jul 1, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

str4d

laanwj

Labels

P2P

Linked (view graph)

#10587 Net: Fix resource leak in ReadBinaryFile(...)#13291 test: Don't include torcontrol.cpp into the test file