Opened 8 years ago

Closed 8 years ago

#5519 closed task (fixed)

Some improvements for torcontrol code (ControlConnection, ControlSocket classes)

Reported by: sebb Owned by: sebb
Priority: Low Milestone:
Component: Archived/Vidalia Version: Vidalia: 0.3.1-alpha
Severity: Keywords:
Cc: sebb, chiiph Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I think it could be nice to build abstract layer for tcp and local sockets to get rid of 'switch (method)' statements, a simple wrapper class could save us some code.
Another thing is to add a destructor, because now allocated sockets are not deleted - they are parent-less and not "delete"d explicitly (I know those are created only once, but still).

Child Tickets

Change History (14)

comment:1 Changed 8 years ago by sebb

Type: defecttask

comment:2 Changed 8 years ago by sebb

Priority: trivialminor
Status: newassigned
Summary: Some improvements for torcontrol code (ControlSocket class)Some improvements for torcontrol code (ControlConnection, ControlSocket classes)

After analyzing the connections between torControl, control socket and control connection objects, I think I've found a case when Control Connection can loop forever, here is possible fail scenario:

1) command is added to _recvQueue: for example GETINFO command waiting
2) tor crashes
3) control socket disconnects -> "disconnected" signal is fired
4) control connection calls onDisconnected() -> status change to "disconnected", isConnected will return "false" from now on
5) torcontrol wants to call "disconnect" manually (in "onStopped" slot), but isConnected() returns false
6) disconnect is neved called, so all recvwaiter objects will wait forever

problem is that "recvQueue" is cleared only in "disconnect" slot of ControlConnection class, so its possible to "turn off" the ControlConnection without clearing it
I think its enough to always clear recvQueue in control connection before attempt to change status to "disconnected" - we can just reuse the code from "disconnect"

comment:3 Changed 8 years ago by chiiph

Nice catch! Would this small patch fix the whole issue?

@@ -241,6 +241,8 @@ ControlConnection::setStatus(Status status)
                                        .arg(statusString(_status))
                                        .arg(statusString(status));
   _status = status;
+  if (_status == Disconnected)
+    disconnect();
 }

I think so, but may be there is a better way of handling this. May be just calling _controlConn->disconnect() regardless of its connection status, and inside it decide to call or not the various disconnect methods *if* connected.

comment:4 Changed 8 years ago by sebb

exactly, I thought about something patching the setStatus method as well
i'll implement other changes i had in mind for this issue + this patch and link a branch here, maybe later today

comment:5 in reply to:  4 Changed 8 years ago by sebb

Replying to sebb:

exactly, I thought about something patching the setStatus method as well
i'll implement other changes i had in mind for this issue + this patch and link a branch here, maybe later today

(a typo: "something" should be removed from first sentence)

comment:6 Changed 8 years ago by sebb

Status: assignedneeds_review

comment:7 Changed 8 years ago by chiiph

Status: needs_reviewneeds_revision

I like the improvements, although you should separate them in two different commits and changes file. One of them is the infinite loop, and the other one is a cleanup.

On a different note, have you experienced a bug where Vidalia issues a GETCONF ORPort before being authenticated? That's a check that is never done in these classes. I'm trying to catch that bug inside the code and come up with a fix, I wanted to let you know so you keep your eye up for this case.

comment:8 Changed 8 years ago by chiiph

Actually the problem seems to be that sometimes, tor responds to PROTOCOLINFO before Vidalia adds a ReceiveWaiter to _recvQueue, I've added a warning msg to this method:

bool
ControlConnection::send(const ControlCommand &cmd,
                        ControlReply &reply, QString *errmsg)
{
  bool result = false;
  QString errstr;

  if (send(cmd, &errstr)) {
    /* Create and enqueue a new receive waiter */
    ReceiveWaiter *w = new ReceiveWaiter();
    _recvQueue.enqueue(w);

    /* Wait for and get the result, clean up, and return */
    qWarning() << "WAITING1";;

    result = w->getResult(&reply, &errstr);

And I see this in the log:

May 25 17:31:59.014 [debug] QtDebugMsg: torcontrol: Control Command: PROTOCOLINFO 1
May 25 17:31:59.015 [debug] QtDebugMsg: torcontrol: Control Reply: 250 PROTOCOLINFO 1
250 AUTH METHODS=HASHEDPASSWORD
250 VERSION Tor="0.2.3.12-alpha"
250 OK
May 25 17:31:59.036 [notice] QtWarningMsg: WAITING1

So Vidalia stays waiting for the result forever. One solution would be to do this:

bool
ControlConnection::send(const ControlCommand &cmd,
                        ControlReply &reply, QString *errmsg)
{
  bool result = false;
  QString errstr;

  /* Create and enqueue a new receive waiter */
  ReceiveWaiter *w = new ReceiveWaiter();
  _recvQueue.enqueue(w);

  if (send(cmd, &errstr)) {
    /* Wait for and get the result, clean up, and return */
    qWarning() << "WAITING1";;

    result = w->getResult(&reply, &errstr);
    qWarning() << "WAITING2";

    if (!result)
      tc::error("Failed to receive control reply: %1").arg(errstr);
    delete w;
  } else {
    tc::error("Failed to send control command (%1): %2").arg(cmd.keyword())
                                                        .arg(errstr);
    _recvQueue.dequeue();
  }

  if (!result && errmsg)
    *errmsg = errstr;
  return result;
}

But I'm wondering if this is not another issue with this particular case of error.

}}}

comment:9 Changed 8 years ago by sebb

Status: needs_revisionneeds_review

first commit for infinite loop fix:
https://github.com/sebthestampede/vidalia/commit/6205ea37dc834390f4a21be8ad75939e7827064a
(same branch name)
what about second "changes" file name, for the ControlSocket cleanup ? the ticket number is the same

I never saw that bug with PROTOCOLINFO, but your fix looks ok to me.
Another thing is, shouldn't we remove deleted RecvWaiter object from the _recvQueue ? Now it looks like a dangling pointer stays in there (after "delete w;" line). And call "delete" on object returned by _recvQueue.dequeue();

comment:10 in reply to:  9 Changed 8 years ago by chiiph

Replying to sebb:

first commit for infinite loop fix:
https://github.com/sebthestampede/vidalia/commit/6205ea37dc834390f4a21be8ad75939e7827064a
(same branch name)
what about second "changes" file name, for the ControlSocket cleanup ? the ticket number is the same

You can either use a .2 suffix for the file name, or just put the two changelog entries in the same file. Whatever you like best.
The important part in here is to make both changes explicit for the person that might look at the commit log or the changelog.

I never saw that bug with PROTOCOLINFO, but your fix looks ok to me.
Another thing is, shouldn't we remove deleted RecvWaiter object from the _recvQueue ? Now it looks like a dangling pointer stays in there (after "delete w;" line). And call "delete" on object returned by _recvQueue.dequeue();

Hm, actually no. The "delete w" should be for both branches of the if. In the first case, the waiter is dequeued when the answer is found in onReadyRead, otherwise the getResult call wouldn't return.
I'd like to get this code clearer too, but I'm not entirely sure if there is a better way to go here.

comment:11 Changed 8 years ago by sebb

Second commit:
https://github.com/sebthestampede/vidalia/commit/49c2f16a4ac4776544f076f954e701b2c8081a17

Ok, now I see, it was not clear to me at first glance.
Anyway, this code seems to work, so I guess we can leave it as it is for now.

Are you planning to include your patch for the PROTOCOLINFO bug ? It could be good to have it merged before I update the "bootstrap test" branch.

comment:12 Changed 8 years ago by chiiph

Every issue discussed in here has been solved and your branch has been merged. It will be out with 0.3.2 alpha.

Should we close this? or do you have any other fix in mind?

comment:13 Changed 8 years ago by sebb

great, I think we can close this ticket.

comment:14 Changed 8 years ago by chiiph

Resolution: fixed
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.