Opened 2 years ago

Closed 2 years ago

#22221 closed defect (fixed)

CID 1405983 test_channelpadding.c dead code

Reported by: catalyst Owned by:
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: mikeperry Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Coverity found this dead code in test_channelpadding.c recently. It looks like the CHANNELS_TO_TEST/3 loop never runs because the CHANNELS_TO_TEST/2 loop runs before it? Is the right thing to do here to switch the sequence of those two loops?

299    /* This loop should add timers to the first position in the timerslot
300     * array, since its timeout is before all other timers. */
   	at_least: At condition i < 33, the value of i must be at least 50.
   	dead_error_condition: The condition i < 33 cannot be true.
301    for (; i < CHANNELS_TO_TEST/3; i++) {
   	
CID 1405983 (#1 of 1): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement: chans[i]->next_padding_time....
302      chans[i]->next_padding_time_ms = monotime_coarse_absolute_msec() + 1;
303      decision = channelpadding_decide_to_pad_channel(chans[i]);
304      tt_int_op(decision, OP_EQ, CHANNELPADDING_PADDING_SCHEDULED);
305      tt_assert(chans[i]->pending_padding_callback);
306      tt_int_op(tried_to_write_cell, OP_EQ, 0);
307    }}}}

Child Tickets

Change History (4)

comment:1 Changed 2 years ago by catalyst

Proposed patch in https://gitlab.com/argonblue/tor/merge_requests/10
mikeperry, would you please comment on whether this reordering will cause a problem for the test case?

comment:2 Changed 2 years ago by catalyst

Status: newneeds_review

comment:3 Changed 2 years ago by mikeperry

Status: needs_reviewmerge_ready

Ah, yes. This test was originally written to exercise a timer layer that limited the number of pending libevent timers. Since Nick added the timing wheel code, that layer was no longer needed and was removed. But it is still a good idea to exercise adding timers out of order, and adding timers with firing times before the currently scheduled batch. Fixing this loop order makes sure that happens, so this is good to go.

comment:4 Changed 2 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged; thanks!

Note: See TracTickets for help on using tickets.