Opened 7 months ago

Closed 7 months ago

#24927 closed defect (fixed)

if (n_chan_id) in circuit_build_failed() can't fail

Reported by: arma Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version: Tor: 0.2.4.4-alpha
Severity: Normal Keywords: review-group-31
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

In circuit_build_failed() we have

    const char *n_chan_id = circ->cpath->extend_info->identity_digest;
[...]
    if (n_chan_id && !already_marked) {

But extend_info->identity_digest is

  char identity_digest[DIGEST_LEN];

So n_chan_id is never going to be NULL.

Child Tickets

Change History (8)

comment:1 Changed 7 months ago by arma

I think this bug is harmless, in that we don't actually rely on circ->cpath->extend_info->identity_digest being non-zero (i.e. not all zeros) for the things that we call.

But it would be good to get a second set of eyes on this conclusion. :)

Last edited 7 months ago by arma (previous) (diff)

comment:2 Changed 7 months ago by arma

(Bug went in during commit 32337502 back in 0.2.4 times.)

comment:3 Changed 7 months ago by nickm

Owner: set to nickm
Status: newaccepted

I think you're right: the only call here that matters is connection_ap_fail_onehop(), which will get called with failed_digest set to an all-zero identity. And if that happens, and the digest matches, we will still compare on address:port instead, since we check for digest_is_zero().

Because of that check in connection_ap_fail_onehop(), I think we should just remove the test here. I'm turning it into an assertion, and renaming the variable a bit.

comment:4 Changed 7 months ago by nickm

See branch bug24927 in my public repository.

comment:5 Changed 7 months ago by nickm

Status: acceptedneeds_review

comment:6 Changed 7 months ago by nickm

Keywords: review-group-31 added

comment:7 Changed 7 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

lgtm;

comment:8 Changed 7 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merged!

Note: See TracTickets for help on using tickets.