Net: Improvements to Tor control port parser #10408

str4d · 2017-05-16T06:23:54Z

Cherry-picked from the following Zcash PRs:

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

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

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 zcash/zcash#2251

laanwj · 2017-05-17T08:09:07Z

src/torcontrol.cpp

+        // Check for reading errors so we don't return any data if we couldn't
+        // read the entire file (or up to maxsize)
+        if (ferror(f))
+            return std::make_pair(false,"");


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

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.

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.

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 · 2017-05-17T08:14:03Z

Thanks for the improvements; I still have to review the unescaping loop in detail but overall it looks good to me.

laanwj · 2017-05-18T17:58:02Z

utACK 49a199b

49a199b torcontrol: Handle escapes in Tor QuotedStrings (Jack Grigg) 0182a11 torcontrol: Log invalid parameters in Tor reply strings where meaningful (Jack Grigg) 0b6f40d torcontrol: Check for reading errors in ReadBinaryFile (Jack Grigg) d63677b torcontrol: Fix ParseTorReplyMapping (Jack Grigg) 29f3c20 torcontrol: Add unit tests for Tor reply parsers (Jack Grigg) d8e03c0 torcontrol: Improve comments (Jack Grigg) Tree-SHA512: aa3ce8072d20299b38c4ba9471af7fab1f5df096c237bf40a96ee9274a357f7366f95ced0cc80f8da1f22f6455a1a8e68bad9a5ff71817eef3397b6aefcbc7ae

Introduced in 0b6f40d via PR bitcoin#10408.

Net: Fix resource leak in ReadBinaryFile(...) Introduced in 3290567 via PR #2177. Cherry-picked from Bitcoin PR bitcoin/bitcoin#10408

Introduced in 0b6f40d via PR bitcoin#10408.

97c112d Declare TorReply parsing functions in torcontrol_tests (Ben Woosley) Pull request description: These methods are standalone string parsing methods which were included into test via an include of torcontrol.cpp, which is bad practice. ~~Splitting them out reveals that they were the only torcontrol.cpp methods under test, so the test file is renamed tor_reply_tests.cpp.~~ Introduced in #10408 Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac

49a199b torcontrol: Handle escapes in Tor QuotedStrings (Jack Grigg) 0182a11 torcontrol: Log invalid parameters in Tor reply strings where meaningful (Jack Grigg) 0b6f40d torcontrol: Check for reading errors in ReadBinaryFile (Jack Grigg) d63677b torcontrol: Fix ParseTorReplyMapping (Jack Grigg) 29f3c20 torcontrol: Add unit tests for Tor reply parsers (Jack Grigg) d8e03c0 torcontrol: Improve comments (Jack Grigg) Tree-SHA512: aa3ce8072d20299b38c4ba9471af7fab1f5df096c237bf40a96ee9274a357f7366f95ced0cc80f8da1f22f6455a1a8e68bad9a5ff71817eef3397b6aefcbc7ae

Introduced in 0b6f40d via PR bitcoin#10408.

49a199b torcontrol: Handle escapes in Tor QuotedStrings (Jack Grigg) 0182a11 torcontrol: Log invalid parameters in Tor reply strings where meaningful (Jack Grigg) 0b6f40d torcontrol: Check for reading errors in ReadBinaryFile (Jack Grigg) d63677b torcontrol: Fix ParseTorReplyMapping (Jack Grigg) 29f3c20 torcontrol: Add unit tests for Tor reply parsers (Jack Grigg) d8e03c0 torcontrol: Improve comments (Jack Grigg) Tree-SHA512: aa3ce8072d20299b38c4ba9471af7fab1f5df096c237bf40a96ee9274a357f7366f95ced0cc80f8da1f22f6455a1a8e68bad9a5ff71817eef3397b6aefcbc7ae

Introduced in 0b6f40d via PR bitcoin#10408.

a9c922b Net: Fix resource leak in ReadBinaryFile(...) (practicalswift) Pull request description: Straightforward fix. Coming from upstream [10587](bitcoin#10587) ``` Potential file descriptor leak introduced in commit 0b6f40d via PR bitcoin#10408 (submitted 2017-05-16, merged 2017-05-18). ``` ACKs for top commit: Fuzzbawls: ACK a9c922b random-zebra: utACK a9c922b and merging... Tree-SHA512: d9fdf1f04e73bb2118a2ad88113d2f4676dedba8668fc1396f446689000f1a75a8bb678f5f84cb7be20781bde1932f2ffe8606666fd371d00d2d5228077350c6

… file 97c112d Declare TorReply parsing functions in torcontrol_tests (Ben Woosley) Pull request description: These methods are standalone string parsing methods which were included into test via an include of torcontrol.cpp, which is bad practice. ~~Splitting them out reveals that they were the only torcontrol.cpp methods under test, so the test file is renamed tor_reply_tests.cpp.~~ Introduced in bitcoin#10408 Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac

str4d added 6 commits May 16, 2017 18:21

torcontrol: Improve comments

d8e03c0

torcontrol: Add unit tests for Tor reply parsers

29f3c20

torcontrol: Fix ParseTorReplyMapping

d63677b

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

torcontrol: Check for reading errors in ReadBinaryFile

0b6f40d

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

torcontrol: Log invalid parameters in Tor reply strings where meaningful

0182a11

jonasschnelli added the P2P label May 16, 2017

laanwj reviewed May 17, 2017

View reviewed changes

laanwj merged commit 49a199b into bitcoin:master May 18, 2017

practicalswift added a commit to practicalswift/bitcoin that referenced this pull request Jun 14, 2017

Net: Fix resource leak in ReadBinaryFile(...)

f2fb132

Introduced in 0b6f40d via PR bitcoin#10408.

practicalswift mentioned this pull request Jun 14, 2017

Net: Fix resource leak in ReadBinaryFile(...) #10587

Merged

str4d deleted the torcontrol-parser-patches branch June 23, 2017 06:45

str4d mentioned this pull request Jun 23, 2017

Net: Fix resource leak in ReadBinaryFile(...) zcash/zcash#2464

Merged

zkbot added a commit to zcash/zcash that referenced this pull request Jul 10, 2017

Auto merge of #2464 - str4d:torcontrol-rbf-resource-leak, r=daira

90a255a

Net: Fix resource leak in ReadBinaryFile(...) Introduced in 3290567 via PR #2177. Cherry-picked from Bitcoin PR bitcoin/bitcoin#10408

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Mar 6, 2018

Net: Fix resource leak in ReadBinaryFile(...)

1161043

Introduced in 0b6f40d via PR bitcoin#10408.

This was referenced May 24, 2018

lint: Add linter to error on #include <*.cpp> #13301

Merged

test: Don't include torcontrol.cpp into the test file #13291

Merged

lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jun 28, 2019

Net: Fix resource leak in ReadBinaryFile(...)

36835a3

Introduced in 0b6f40d via PR bitcoin#10408.

furszy pushed a commit to furszy/bitcoin-core that referenced this pull request Apr 2, 2020

Net: Fix resource leak in ReadBinaryFile(...)

a9c922b

Introduced in 0b6f40d via PR bitcoin#10408.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Net: Improvements to Tor control port parser #10408

Net: Improvements to Tor control port parser #10408

Uh oh!

str4d commented May 16, 2017

Uh oh!

laanwj May 17, 2017 •

edited

Loading

Uh oh!

str4d May 17, 2017

Uh oh!

str4d May 17, 2017

Uh oh!

laanwj May 17, 2017 •

edited

Loading

Uh oh!

laanwj commented May 17, 2017

Uh oh!

laanwj commented May 18, 2017

Uh oh!

Uh oh!

Net: Improvements to Tor control port parser #10408

Net: Improvements to Tor control port parser #10408

Uh oh!

Conversation

str4d commented May 16, 2017

Uh oh!

laanwj May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

str4d May 17, 2017

Choose a reason for hiding this comment

Uh oh!

str4d May 17, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 17, 2017

Uh oh!

laanwj commented May 18, 2017

Uh oh!

Uh oh!

laanwj May 17, 2017 •

edited

Loading

laanwj May 17, 2017 •

edited

Loading