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

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.

changed milestone to %Tor: 0.3.3.x-final

added component::core tor/tor milestone::Tor: 0.3.3.x-final owner::nickm priority::medium resolution::fixed review-group-31 reviewer::dgoulet severity::normal status::closed type::defect version::tor 0.2.4.4-alpha labels

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. :)

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

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.

Trac:
Status: new to accepted
Owner: N/A to nickm

See branch bug24927 in my public repository.

Trac:
Status: accepted to needs_review

Trac:
Keywords: N/A deleted, review-group-31 added

lgtm;

Trac:
Status: needs_review to merge_ready
Reviewer: N/A to dgoulet

Merged!

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

moved to tpo/core/tor#24927 (closed)