Opened 2 years ago

Last modified 15 months ago

#21716 new defect

Avoid recursive call to routerlist_remove_old_routers via router_rebuild_store

Reported by: anstein Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: easy, intro, 034-triage-20180328, 034-removed-20180328
Cc: Actual Points:
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

Periodic functions have their log output duplicated when using adb logcat:

orbot/external/tor/src/or/periodic.c

static void
periodic_event_dispatch(evutil_socket_t fd, short what, void *data)
{

(void)fd;
(void)what;
periodic_event_item_t *event = data;

time_t now = time(NULL);
const or_options_t *options = get_options();

log_debug(LD_GENERAL, "Dispatching %s", event->name);

int r = event->fn(now, options);

I did uncomment the above log_debug which is then printed only once as expected.
The problem starts with r=events->fn(now, options);. Every function called this way seems to have its log output duplicated. I checked with builtin atomics that the function is actually called only once but the log output is duplicated, e.g.:

03-12 20:45:22.627 17948 17948 D Tor : periodic_event_dispatch(): Dispatching check_descriptor
03-12 20:45:22.637 17948 17948 I Tor : routerlist_remove_old_routers(): We have 0 live routers and 0 old router descriptors.
03-12 20:45:22.637 17948 17948 I Tor : routerlist_remove_old_routers(): We have 0 live routers and 0 old router descriptors.

This is somewhat irritating when tracing the flow of events.

Child Tickets

Change History (11)

comment:1 Changed 2 years ago by teor

Component: Applications/OrbotCore Tor/Tor
Keywords: easy intro added
Milestone: Tor: 0.3.2.x-final
Owner: n8fr8 deleted
Points: 0.5
Status: newassigned
Summary: duplicate log outputAvoid recursive call to routerlist_remove_old_routers via router_rebuild_store

This is not a duplicate logging bug, and it's in tor, not orbot.

It is due to a recursive call to routerlist_remove_old_routers via router_rebuild_store.

Still, we should fix it eventually, because it's confusing, and recursive calls lead to subtle bugs and complex call graphs.

comment:2 Changed 2 years ago by nickm

Status: assignednew

Change the status of all assigned/accepted Tor tickets with owner="" to "new".

comment:3 Changed 22 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

comment:4 Changed 17 months ago by aruna1234

I think it's the other way round. There are two call to router_rebuild_store() from routerlist_remove_old_routers(), compared to a single call in router_rebuild_store() to routerlist_remove_old_routers. Is there any confusion?

comment:5 Changed 17 months ago by teor

We need to pick one of these functions, and make it call the other one using a loop. It's much simpler to do it that way.

Here's some background reading on recursion and iteration (loops). It might help explain why we prefer loops:
https://en.m.wikipedia.org/wiki/Recursion_(computer_science)#Recursion_versus_iteration

comment:6 Changed 17 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Defer a small handful of non-enhancement "new" tickets to 0.3.4

comment:7 Changed 17 months ago by aruna1234

It's not a typical recursion, where the function calls itself, but is surely an issue, as this:

if (!(flags & RRS_DONT_REMOVE_OLD))

routerlist_remove_old_routers();

leads to a call to routerlist_remove_old_routers(), which in turn leads to two calls to the calling function.

Maybe a while loop replacing the if condition would resolve this?

while(!(flags & RRS_DONT_REMOVE_OLD))...

make check ran without fail.

comment:8 Changed 17 months ago by teor

No, an infinite loop is not a solution to indirect recursion.
The code structure needs to be changed to avoid functions calling themselves indirectly.

comment:9 Changed 15 months ago by nickm

Keywords: 034-triage-20180328 added

comment:10 Changed 15 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:11 Changed 15 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

Note: See TracTickets for help on using tickets.