#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:
Severity: Normal Keywords: review-group-31
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:


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 10 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 for the things that we call.

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

Version 0, edited 10 months ago by arma (next)

comment:2 Changed 10 months ago by arma

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

comment:3 Changed 10 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 10 months ago by nickm

See branch bug24927 in my public repository.

comment:5 Changed 10 months ago by nickm

Status: acceptedneeds_review

comment:6 Changed 10 months ago by nickm

Keywords: review-group-31 added

comment:7 Changed 10 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready


comment:8 Changed 10 months ago by nickm

Resolution: fixed
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.