Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#6028 closed defect (fixed)

errno_to_orconn_end_reason() spams log when errno == 0

Reported by: ln5 Owned by:
Priority: Low Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by andrea)

Should we make errno_to_orconn_end_reason() shut up when errno is 0?
Or should we rather make sure not to call it when errno is 0?

Child Tickets

Change History (13)

comment:1 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

Under what circumstances is it 0?

If 0 is a common input, we should treat it as whatever the end reason for "misc" or "internal" is, I guess.

comment:2 Changed 8 years ago by ln5

I think it's circuit_extend() calling connection_or_connect() which
then gets -1 from connection_connect().

There are two obious ways out of there with return code -1 and no
attempt to set *socket_error -- too many open connections and when XXX
fails creating a bufferevent. Judging from other log lines I'm
suffering from the first. Ending up with
END_OR_CONN_REASON_RESOURCE_LIMIT might be the right thing in this
case. Should we *socket_error=ENOBUFS in connection_connect() after
the warn_too_many_conns() call?

comment:3 in reply to:  2 Changed 8 years ago by ln5

Replying to ln5:

There are two obious ways out of there with return code -1 and no
attempt to set *socket_error -- too many open connections and when XXX
fails creating a bufferevent. Judging from other log lines I'm

s/XXX/connection_add_impl()/1

comment:4 Changed 7 years ago by andrea

Description: modified (diff)

The function errno_to_orconn_end_reason() is called from four places:

  • connection_or_connect() (connect_or.c, line 1112):

This has someting like:

int socket_error = 0;

if ( connection_connect( ..., &socket_error ) == -1 ) {
    ... invokes errno_to_orconn_end_reason(socket_error)
}

So, as ln5 stated above, this can happen if connection_connect() returns -1 without setting socket_error.

  • connection_handle_read_impl() (connect.c, line 2671):

Similar to connection_or_connect(); this can happen if connection_read_to_buf()
returns < 0 without setting socket_error.

  • connection_handle_event_cb() (connect.c, line 3020):

The value of socket_error comes from evutil_socket_geterror( conn->s ) if
event & BEV_EVENT_ERROR; can this ever happen without evutil_socket_geterror() returning
an error?

  • connection_handle_write_impl() (connect.c, line 3195):

This has something of the form:

if ( e ) {
    ...;
    connection_or_connect_failed( ...,
        errno_to_orconn_end_reason(e));
}

so it looks impossible for it to be caused here.

comment:5 Changed 7 years ago by andrea

Description: modified (diff)

comment:6 Changed 7 years ago by andrea

I think the only path out of connection_handle_read_impl() without setting *socket_error should also log_warn(LD_BUG, "apparently, reading pending bytes can fail."). Since no such message was mentioned I'll assume this is not the case.

comment:7 Changed 7 years ago by andrea

What I can gather of libevent docs sure makes it sound like evutil_socket_geterror() won't ever return 0.

comment:8 Changed 7 years ago by andrea

It looks like ln5 is right about the two possible ways to get out of connection_connect() returning -1 without setting *socket_error. Ending up with END_OR_CONN_REASON_RESOURCE_LIMIT by setting ENOBUFS in the first case sounds reasonable; for the second, perhaps the same? Bufferevents would only fail to be created because of memory shortage or something like that?

comment:10 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks correct to me. Merging. Thanks!

comment:11 Changed 7 years ago by nickm

Oops. Looks like I thought I thought I merged this, but didn't. Trying again.

comment:12 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:13 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.