Opened 5 years ago

Closed 3 months ago

#13221 closed defect (fixed)

Misleading error messages about bind_ipv4_only and bind_ipv6_only?

Reported by: arma Owned by:
Priority: Low Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.9-alpha
Severity: Normal Keywords: 041-proposed, tor-client, easy, logging, message, usability, 035-backport, 040-backport, consider-backport-after-0404
Cc: ln5, jeroen@…, katterjohn@… Actual Points: .1
Parent ID: Points: .1
Reviewer: teor Sponsor:

Description

      if (bind_ipv4_only && tor_addr_family(&addr) == AF_INET6) {
        log_warn(LD_CONFIG, "Could not interpret %sPort address as IPv6",
                 portname);
        goto err;
      }

Is this warn mixed up? Same with the one below it.

Child Tickets

Change History (18)

comment:1 Changed 5 years ago by ln5

Cc: ln5 added

comment:2 Changed 5 years ago by massar

Cc: jeroen@… added

Depending on context, that should likely say "Only binding on IPv4 while IPv6 address provided"

comment:3 Changed 5 years ago by teor

Keywords: lorax added
Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

comment:4 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:5 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:6 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:7 Changed 2 years ago by nickm

Keywords: easy logging message usability added; lorax removed
Points: .1
Severity: Normal

comment:8 Changed 6 months ago by kjak

Keywords: 041-proposed added
Status: newneeds_review

Here is a PR to fix this: https://github.com/torproject/tor/pull/669

I didn't change the overall wording of the error message because personally I think it's fine (aside from the IP version mixup). I think it's short and clear and means "You told me to only bind to an IPv4 address but I couldn't interpret the address as IPv4."

I took the liberty of also cleaning up the error-checking logic to make it a little more precise about what is being checked. I hope that's OK.

(I'm also adding the keyword 041-proposed, which my other recent tickets got. Is it OK for me to do that or is adding a keyword like that reserved for core team members?)

comment:9 Changed 5 months ago by dgoulet

Reviewer: teor

comment:10 in reply to:  8 ; Changed 5 months ago by teor

Actual Points: .1
Keywords: 029-backport-maybe 034-backport-maybe 035-backport 040-backport added
Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Status: needs_reviewneeds_revision
Version: Tor: 0.2.3.9-alpha

Hi,

Thanks for this pull request.

Sorry it's taken a while for us to get to it: many of us were at a hackfest in January, then on leave.

Replying to kjak:

Here is a PR to fix this: https://github.com/torproject/tor/pull/669

I didn't change the overall wording of the error message because personally I think it's fine (aside from the IP version mixup). I think it's short and clear and means "You told me to only bind to an IPv4 address but I couldn't interpret the address as IPv4."

I took the liberty of also cleaning up the error-checking logic to make it a little more precise about what is being checked. I hope that's OK.

It looks good to me.

(I'm also adding the keyword 041-proposed, which my other recent tickets got. Is it OK for me to do that or is adding a keyword like that reserved for core team members?)

Yes, that's fine. We're still experimenting with our proposed tickets process.

Review:

There was a failure in one of the jobs due to our stochastic random tests.
I re-ran the job and it was fine.

This patch fixes an error that's been around since 2011.
It's confusing to relay operators, so it is a backport candidate.

Would you mind rebasing your patch on our maint-0.2.9 branch?
If you can't, just let us know, and we'll do it eventually.

comment:11 in reply to:  10 Changed 5 months ago by kjak

Cc: katterjohn@… added
Status: needs_revisionneeds_review

Replying to teor:

Would you mind rebasing your patch on our maint-0.2.9 branch?
If you can't, just let us know, and we'll do it eventually.

This taught me that my git-fu is even weaker than I thought it was, but it looks like I managed to do it. I rebased on maint-0.2.9 and here is a new PR:

https://github.com/torproject/tor/pull/726

comment:12 Changed 5 months ago by teor

Status: needs_reviewmerge_ready

Thanks! Looks good to me, and CI likes it.

comment:13 in reply to:  12 Changed 5 months ago by kjak

Awesome. Thanks for reviewing!

comment:14 Changed 5 months ago by nickm

Milestone: Tor: 0.4.1.x-finalTor: 0.3.5.x-final

merged to 0.4.0 and forward; marking for possible backport

comment:15 Changed 4 months ago by teor

Keywords: consider-backport-after-0404-alpha added

These tickets are low-risk changes that were marked for backport after the release of 0.4.0.2-alpha. I want them to get testing in at least one alpha release, so I'll look at them after 0.4.0.4-alpha is released.

comment:16 Changed 3 months ago by teor

Keywords: consider-backport-after-0404 added; consider-backport-after-0404-alpha removed

Drop the -alpha from backport tags

comment:17 Changed 3 months ago by teor

Keywords: 029-backport-maybe 034-backport-maybe removed

UX issues only go back to the latest LTS.

comment:18 Changed 3 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged to 0.3.5 and merged forward.

Merged #23790, #29665, #29017, #27199, #29144, #13221, #28698.

Note: See TracTickets for help on using tickets.