Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#21329 closed defect (fixed)

onions/detached and onions/current should return empty lists instead of errors

Reported by: meejah Owned by:
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Archived/Stem Version: Tor:
Severity: Normal Keywords: controller
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


If you do a GETINFO query on onions/detached or onions/current and there aren't any, a 551 error is returned. Most other things just return an empty list/value in this case and I think these should too.

One consequence of this is that if you ask for onions/current with some other keys and there are no current onions, the whole query will fail.

So GETINFO onions/current should return onions/current= instead of an error.

For example you can try carml cmd getinfo orconn-status onions/current to see this behavior.

Child Tickets

Change History (18)

comment:1 Changed 4 years ago by nickm

Milestone: Tor: unspecified

Makes sense to me; a patch would be welcome.

comment:3 Changed 4 years ago by dgoulet

Milestone: Tor: unspecifiedTor: 0.3.1.x-final
Status: newneeds_review

comment:4 Changed 4 years ago by nickm

Status: needs_reviewmerge_ready

Looks correct to me!

comment:5 Changed 4 years ago by nickm

Keywords: review-group-16 added

comment:6 Changed 4 years ago by nickm

Keywords: review-group-16 removed
Resolution: implemented
Status: merge_readyclosed


comment:7 Changed 4 years ago by atagar

Resolution: implemented
Status: closedreopened

Gonna reopen. Recent push for this broke Stem's integ tests...

FAIL: test_without_ephemeral_hidden_services
Traceback (most recent call last):
  File "/srv/jenkins-workspace/workspace/stem-tor-ci/test/", line 126, in wrapped
    return func(self, *args, **kwargs)
  File "/srv/jenkins-workspace/workspace/stem-tor-ci/test/", line 143, in wrapped
    return func(self, *args, **kwargs)
  File "/srv/jenkins-workspace/workspace/stem-tor-ci/test/integ/control/", line 569, in test_without_ephemeral_hidden_services
    self.assertEqual([], controller.list_ephemeral_hidden_services())
AssertionError: Lists differ: [] != ['']

Second list contains 1 additional elements.
First extra element 0:

- []
+ ['']

comment:8 Changed 4 years ago by meejah

Looking quickly at Stem's code, I think this is an integration test bug.

That is, I think list_ephemeral_hidden_services should check if it got an empty answer from Tor (similar to other GETINFO empty replies).

That said, I also don't see how that test ever passed since it appears it would re-raise the 552/protocol-error exception that 'getinfo onions/current' does without the patch in this ticket.

comment:9 Changed 4 years ago by atagar

Thanks, I'll dig a little more on my end to figure out exactly what about tor's wire response changed and which side the bug lays on. Might not be able to get to this until after the weekend (visiting family, and can no longer build new tor versions on my netbook since it's pretty out of date).

comment:10 Changed 4 years ago by nickm

Component: Core TorCore Tor/Tor

comment:11 Changed 4 years ago by atagar

Ok, took a quick peek. Change is the following...


>>> GETINFO onions/current
551 No onion services of the specified type.

>>> GETINFO onions/current
250 OK

Both responses are valid according to the spec and it doesn't specify what's done when no hidden services exist. Stem specifically looks for the old 551 response, hence the discrepancy.

I wouldn't mind changing stem to recognize the new behavior but this tor change may break folks who presently call list_ephemeral_hidden_services(). So it's a tradeoff - is this behavior change worth that breakage.

Up to you, Nick.

comment:12 Changed 4 years ago by teor

Maybe the stem list processing could be tweaked?

(That is, why is there a semantic difference between an empty list, and a list containing an empty string? Neither is a valid ephemeral onion service.)

comment:13 Changed 4 years ago by atagar

Hi teor. I'm more than happy to change Stem to recognize the new behavior. The point I was raising is that existing Stem releases will have an issue.

Honestly this is a pretty minor bug. It's an edge case for ephemeral hidden services so I don't see this biting many people. I'd be happy to go either way (change tor and risk a minor breakage for stem users, or keep tor as-is).

Nick: Any preference?

Last edited 4 years ago by atagar (previous) (diff)

comment:14 Changed 4 years ago by meejah

atagar: this breaks multi-key GETINFO queries, though; all other "empty things" in the control protocol return an empty-list like that.

So, if you do GETINFO info/names onions/current then you'll get a 551 and the whole thing fails (instead of, for all other cases, getting two answers)...

comment:15 Changed 4 years ago by atagar

Component: Core Tor/TorCore Tor/Stem

Gotcha. Personally I find that a compelling reason to go with this change. As mentioned before it's very much an edge case for Stem users and fixing this sounds worth it to me.

Reassigning this to myself to tweak Stem to handle the new responses. Thanks meejah!

comment:16 Changed 4 years ago by atagar

Resolution: fixed
Status: reopenedclosed

comment:17 Changed 3 years ago by meejah

Version: Tor:

comment:18 Changed 3 years ago by meejah

I believe this bug first existed in and commit 915c7438a77edfaf3103b69cb494a4f069a79a0c

Note: See TracTickets for help on using tickets.