Opened 7 months ago

Closed 7 months ago

Last modified 7 months ago

#33674 closed task (fixed)

Check return value handling in the relay and dirauth stubs

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt
Cc: Actual Points: 0.4
Parent ID: Points: 0.5
Reviewer: nickm Sponsor:

Description

In #33668, we discovered a relay module stub that didn't handle an int *have_low_ports_out return value correctly.

We should check all the relay and dirauth module return values, including pointer out and inout return values.

Child Tickets

TicketStatusOwnerSummaryComponent
#33668closednickm--disable-module-relay yields to a Bug:Core Tor/Tor

Change History (8)

comment:1 Changed 7 months ago by teor

Actual Points: 0.3
Keywords: technical-debt added
Reviewer: nickm
Status: assignedneeds_review

There are a few places where our out argument handling could be risky.

See my PRs:

comment:2 Changed 7 months ago by nickm

LGTM, squashing and merging.

comment:3 Changed 7 months ago by nickm

Status: needs_reviewmerge_ready

I've squashed and merged the 0.4.3 version; I'll wait for more CI to finish on the master version, then I'll cherry-pick its changes.

comment:4 Changed 7 months ago by nickm

Status: merge_readyneeds_revision

Actually, it looks like there is a non-spurious CI warning on the master version:

./src/feature/relay/routerkeys.h:92:36: error: parameter ‘sign_out’ set but not used [-Werror=unused-but-set-parameter]
                               int *sign_out)
                                    ^~~~~~~~
./src/feature/relay/routerkeys.h: In function ‘make_tap_onion_key_crosscert’:
./src/feature/relay/routerkeys.h:106:35: error: parameter ‘len_out’ set but not used [-Werror=unused-but-set-parameter]
                              int *len_out)
                                   ^~~~~~~

(The 0.4.3 branch is already merged)

comment:5 in reply to:  4 Changed 7 months ago by teor

Actual Points: 0.30.4
Keywords: 043-must removed
Milestone: Tor: 0.4.3.x-finalTor: 0.4.4.x-final

Replying to nickm:

Actually, it looks like there is a non-spurious CI warning on the master version:

./src/feature/relay/routerkeys.h:92:36: error: parameter ‘sign_out’ set but not used [-Werror=unused-but-set-parameter]
                               int *sign_out)
                                    ^~~~~~~~
./src/feature/relay/routerkeys.h: In function ‘make_tap_onion_key_crosscert’:
./src/feature/relay/routerkeys.h:106:35: error: parameter ‘len_out’ set but not used [-Werror=unused-but-set-parameter]
                              int *len_out)
                                   ^~~~~~~

(The 0.4.3 branch is already merged)

The master CI is running because I pushed a fix for that issue :-)

Unfortunately, it doesn't show up on my local compiler, so it's been hard to make sure I caught all the bugs.

Once CI passes, anyone can merge:

comment:6 Changed 7 months ago by teor

Status: needs_revisionneeds_review

comment:7 Changed 7 months ago by teor

Resolution: fixed
Status: needs_reviewclosed

CI passed, merged to master. Thanks!

comment:8 Changed 7 months ago by teor

(Actually, I cherry-picked the new commits from master, because the 0.4.3 PR was squashed.)

Note: See TracTickets for help on using tickets.