[tor-bugs] #16990 [Tor Browser]: Circuit visualizer stops working after some time
Tor Bug Tracker & Wiki
blackhole at torproject.org
Wed Feb 3 01:27:46 UTC 2016
#16990: Circuit visualizer stops working after some time
-------------------------------------------------+-------------------------
Reporter: cypherpunks | Owner: tbb-
Type: defect | team
Priority: Medium | Status:
Component: Tor Browser | assigned
Severity: Normal | Milestone:
Keywords: tbb-torbutton, tbb-circuit-display, | Version:
TorBrowserTeam201602 | Resolution:
Parent ID: | Actual Points:
Sponsor: | Points:
-------------------------------------------------+-------------------------
Comment (by cypherpunks):
I bring news, everyone!
Bad news: the bug is present in 5.5 as well.
Worse news: Tor project programmers apparently can't write parsers for
their own protocols.
Better news: I think I discovered at least one trigger for this bug.
I you don't want me to spoil your bug hunting, then just have this and go
dig at the code:
{{{
[01-29 09:48:25] Torbutton INFO: controlPort << getinfo circuit-status
[01-29 09:48:25] Torbutton INFO: controlPort >> 250+circuit-status=
243 BUILT $xxxxxx~nnnn,$xxxxxx~nnnn,$xxxxxx~nnnn BUNCH=OF KEY=VALUE
PAIRS=HERE
249 BUILT $xxxxxx~nnnn,$xxxxxx~nnnn,$xxxxxx~nnnn BUNCH=OF KEY=VALUE
PAIRS=HERE
247 BUILT $xxxxxx~nnnn,$xxxxxx~nnnn,$xxxxxx~nnnn BUNCH=OF KEY=VALUE
PAIRS=HERE
248 BUILT $xxxxxx~nnnn,$xxxxxx~nnnn,$xxxxxx~nnnn BUNCH=OF KEY=VALUE
PAIRS=HERE
250 BUILT $xxxxxx~nnnn BUNCH=OF KEY=VALUE PAIRS=HERE
}}}
I found that in my logs sometime after I noticed that the display was
gone. Why, yes those are 2 complete log messages, I didn't fuck up my
copypaste. You are right though, the second looks incomplete. Aaaand now
you see it, yes it starts and ends with 250.
This is the last message in the logs from the circuit display controller.
It went silent after this.
I had some time yesterday so I looked at the code. Narrating my
adventures would take too much so I'm just going to paste a diff with the
notes I took while reading. I point at a few places where I think bugs
exist; some of the comments are there just to keep track of the lovely
chained chains of functional filigrane arthuredelstein seems so infatuated
with (I understand you arthur, I like functional programming too, but man,
this is javascript, you know, it's kinda disgusting), so breadcrumbs
basically; also I comment on a couple of unrelated issues. All my
comments are prefixed with "punk:". The diff is against Torbutton as
shipped in 5.5, so 1.9.4.3.
Notice that I didn't live-debug the code, just read, so take what I say
with a boulder of salt.
{{{
--- a/torbutton/modules/tor-control-port.js
+++ b/torbutton/modules/tor-control-port.js
@@ -113,7 +113,10 @@ io.asyncSocket = function (host, port, o
let totalString = pendingWrites.join("");
try {
outputStream.write(totalString, totalString.length);
- log("controlPort << " + aString + "\n");
+ // punk: unrelated: for Firefox's console output,
log()
+ // will write each message in a separate line; for
stderr
+ // output, log() already appends '\n'
+ log("controlPort << " + aString.trim());
} catch (err) {
onError(err);
}
@@ -162,6 +165,21 @@ io.onLineFromOnMessage = function (onMes
// Add to the list of pending lines.
pendingLines.push(line);
// If line is the last in a message, then pass on the full multiline
message.
+ //
+ // punk: bug: this will cut the message short; it happens when one
the
+ // reply lines starts with the same 3 digits as the first one; in the
case
+ // of a successful "circuit-status" command, this happens when one of
the
+ // circuits in the reply has a circuit id of 250
+ //
+ // punk: bug: "pendingLines" can grow indefinetely if the lines
pushed into
+ // it do not align with the expected pattern: think what happens if
the
+ // first line pushed is ".", as could happen thanks to the other bug
just
+ // mentioned; this effectively disables the whole control-port
dispatcher
+ // contraption: no more messages will be received through this
controller
+ //
+ // -> how dangerous is this? what else is this controller used for?
+ //
+ // -> apparently nothing else; good
if (line.match(/^\d\d\d /) &&
(pendingLines.length === 1 ||
pendingLines[0].substring(0,3) === line.substring(0,3))) {
@@ -218,6 +236,21 @@ io.matchRepliesToCommands = function (as
};
// Watch for responses (replies or error messages)
dispatcher.addCallback(/^[245]\d\d/, function (message) {
+ // punk: bug: underflow if the callback triggers before the
sendCommand()
+ // in the following Promise is called (and, therefore, a triple is
pushed
+ // into "commandQueue")
+ //
+ // -> wait! apparently this is not actually an error (wat?):
[].shift()
+ // just returns undefined
+ //
+ // -> but then the _assignment_ fails: throws TypeError
+ //
+ // -> this may be happening in the case of the circuit display fuck-
up: the
+ // reply message is cut short, but the rest of the message (some
circuit
+ // status lines and then '.' and then '250 OK') will still be
delivered by
+ // Firefox through the socket, will be received by the smattering of
+ // chained funclets, and could then be made into a new message that
will be
+ // passed into this function
let [command, replyCallback, errorCallback] = commandQueue.shift();
if (message.match(/^2/) && replyCallback) replyCallback(message);
if (message.match(/^[45]/) && errorCallback) {
@@ -273,6 +306,9 @@ io.controlSocket = function (host, port,
// ## utils
// A namespace for utility functions
+//
+// punk: unrelated: why the fuck are these here? there's even a
"utils.js" over
+// there...
let utils = utils || {};
// __utils.identity(x)__.
@@ -398,6 +434,7 @@ info.keyValueStringsFromMessage = utils.
// and applies transformFunction to each line.
info.applyPerLine = function (transformFunction) {
return function (text) {
+ // punk: text is null, null.trim() throws TypeError
return utils.splitLines(text.trim()).map(transformFunction);
};
};
@@ -433,6 +470,7 @@ info.routerStatusParser = function (valu
// __info.circuitStatusParser(line)__.
// Parse the output of a circuit status line.
info.circuitStatusParser = function (line) {
+ // punk: unrelated: "circuit" here is called "path" in the spec
let data = utils.listMapData(line, ["id","status","circuit"]),
circuit = data.circuit;
// Parse out the individual circuit IDs and names.
@@ -500,11 +538,13 @@ info.getParser = function(key) {
info.stringToValue = function (string) {
// key should look something like `250+circuit-status=` or `250
-circuit-status=...`
// or `250 circuit-status=...`
- let matchForKey = string.match(/^250[ +-](.+?)=/),
+ let matchForKey = string.match(/^250[ \+-](.+?)=/),
key = matchForKey ? matchForKey[1] : null;
if (key === null) return null;
// matchResult finds a single-line result for `250-` or `250 `,
// or a multi-line one for `250+`.
+ //
+ // punk: won't match any of these
let matchResult = string.match(/^250[ -].+?=(.*)$/) ||
string.match(/^250\+.+?=([\s\S]*?)^\.$/m),
// Retrieve the captured group (the text of the value in the key-
value pair)
@@ -521,6 +561,9 @@ info.stringToValue = function (string) {
// __info.getMultipleResponseValues(message)__.
// Process multiple responses to a GETINFO or GETCONF request.
info.getMultipleResponseValues = function (message) {
+ // punk: keyValueStringsFromMessage() won't match the message, the
result
+ // will be an empty array, so all thre rest is a no-op, and the return
value
+ // is also an empty array
return info.keyValueStringsFromMessage(message)
.map(info.stringToValue)
.filter(utils.identity);
@@ -534,6 +577,12 @@ info.getInfo = function (aControlSocket,
}
return aControlSocket
.sendCommand("getinfo " + key)
+ // punk: overflow; getMultipleResponseValues returns an empty array
+ //
+ // -> lol, apparently an overflow is not an error in this ridiculous
+ // language, the result is just undefined
+ //
+ // -> so then info.getInfo returns undefined
.then(response => info.getMultipleResponseValues(response)[0]);
};
@@ -639,4 +688,3 @@ let controller = function (host, port, p
// Export the controller function for external use.
var EXPORTED_SYMBOLS = ["controller"];
-
--- a/torbutton/chrome/content/tor-circuit-display.js
+++ b/torbutton/chrome/content/tor-circuit-display.js
@@ -113,6 +113,7 @@ let nodeDataForCircuit = function* (cont
// Returns the circuit status for the circuit with the given ID.
let getCircuitStatusByID = function* (aController, circuitID) {
let circuitStatuses = yield aController.getInfo("circuit-status");
+ // punk: this test fails: circuitStatuses is undefined
if (circuitStatuses) {
for (let circuitStatus of circuitStatuses) {
if (circuitStatus.id === circuitID) {
@@ -120,6 +121,7 @@ let getCircuitStatusByID = function* (aC
}
}
}
+ // punk: so null is returned
return null;
};
@@ -139,16 +141,20 @@ let collectIsolationData = function (aCo
if (!knownCircuitIDs[streamEvent.CircuitID]) {
logger.eclog(3, "streamEvent.CircuitID: " +
streamEvent.CircuitID);
knownCircuitIDs[streamEvent.CircuitID] = true;
+ // punk: circuitStatus is null, so credentials is null as well
let circuitStatus = yield getCircuitStatusByID(aController,
streamEvent.CircuitID),
credentials = circuitStatus ?
(trimQuotes(circuitStatus.SOCKS_USERNAME) +
"|" +
trimQuotes(circuitStatus.SOCKS_PASSWORD)) :
null;
+ // punk: so the update doesn't happen
if (credentials) {
let nodeData = yield nodeDataForCircuit(aController,
circuitStatus);
credentialsToNodeDataMap[credentials] = nodeData;
updateUI();
}
+ // punk: but then nothing else, no other visible breakage...
shouldn't
+ // you be screaming something here?
}
}).then(null, Cu.reportError));
};
@@ -356,4 +362,3 @@ return setupDisplay;
// Finish createTorCircuitDisplay()
})();
-
}}}
Take-away points:
1. The control protocol doesn't have a formal grammar (no, control-
spec.txt doesn't qualify, it, at the very least, is incomplete). Result:
objectively-correct parsers can't be generated.
2. Tor and the controller in Torbutton do not speak the same protocol.
Expect more breakage.
3. Compartmentalising controllers is a good idea. Keep doing that.
4. JavaScript is ridonkeylous. Keep away.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16990#comment:28>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list