Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#7089 closed defect (fixed)

channel_process_cells: cells handled out of order

Reported by: nickm Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: andrea Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

channel_process_cells handles cells with a SMARTLIST_FOREACH loop that does SMARTLIST_DEL_CURRENT in the middle. But that's not order-preserving. Shouldn't it be?

Child Tickets

Change History (4)

comment:1 Changed 7 years ago by nickm

We should probably audit the other SMARTLIST_DEL_CURRENT uses too

comment:2 Changed 7 years ago by nickm

Status: newneeds_review

From #6465 :

In channel_process_incoming, do we want to process that queue in-order? It seems that doing the DEL_CURRENT approach is likely to have weird consequences. Perhaps it would be better to extract all the members into a separate list (smartlist_add_all(), smartlist_clear()), then process the separate list, then free it.

We even talked about this tonight, and thought that you'd fixed the code. Instead it turns out that the function got renamed.

Please review branch "bug7089" in my public repository.

comment:3 Changed 7 years ago by andrea

Resolution: fixed
Status: needs_reviewclosed

That patch looks like it should be fine; I'll merge it.

comment:4 Changed 7 years ago by andrea

The remaining SMARTLIST_DEL_CURRENT uses in channel.c will be unproblematic, they're in channel_run_cleanup() and channel_listener_run_cleanup(), so they always delete the entire list and those lists are order-insensitive anyway.

Note: See TracTickets for help on using tickets.