Opened 3 years ago

Closed 3 years ago

#20716 closed defect (fixed)

memory leak in connection_handle_listener_read()

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.6.3-alpha
Severity: Normal Keywords: 028-backport 027-backport 026-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: nickm Sponsor:

Description

Running my Tor relay under valgrind with all the config options that Yawning's sandboxed Tor Browser turns on, I see:

==18287== 70,128 bytes in 8,766 blocks are definitely lost in loss record 64 of 64
==18287==    at 0x4C28C20: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==18287==    by 0x62AF989: strdup (strdup.c:42)
==18287==    by 0x26329D: tor_strdup_ (util.c:288)
==18287==    by 0x251013: tor_addr_to_str_dup (address.c:1209)
==18287==    by 0x202245: connection_handle_listener_read (connection.c:1598)
==18287==    by 0x2025BC: connection_handle_read_impl (connection.c:3344)
==18287==    by 0x14B160: conn_read_callback (main.c:694)
==18287==    by 0x53613DB: event_base_loop (in /usr/lib/x86_64-linux-gnu/libevent-2.0.so.5.1.9)
==18287==    by 0x14C1E3: run_main_loop_once (main.c:2385)
==18287==    by 0x14C1E3: run_main_loop_until_done (main.c:2429)
==18287==    by 0x14C1E3: do_main_loop (main.c:2357)
==18287==    by 0x14F934: tor_main (main.c:3486)
==18287==    by 0x147AC8: main (tor_main.c:34)

(This is the Tor from my bug20423 branch, which is commit caf742287 from close to the edge of maint-0.2.9 plus two hopefully irrelevant commits -- so I am calling the version 0.2.9.5-alpha.)

Child Tickets

Change History (16)

comment:1 Changed 3 years ago by arma

Version: Tor: 0.2.9.5-alphaTor: 0.2.6.3-alpha

The problem was commit 8d59ddf3c. Our code now does:

    newconn->port = port;
    newconn->address = tor_addr_to_str_dup(&addr);

[...]

    if (new_type == CONN_TYPE_AP && conn->socket_family == AF_UNIX) {
      newconn->port = 0;
      newconn->address = tor_strdup(conn->address);
      log_info(LD_NET, "New SOCKS AF_UNIX connection opened");
    }

So basically, we leak an 'address' string every time we get a unix domain socket socks request.

comment:2 Changed 3 years ago by icanhasaccount

I made an attempt at a fix: https://github.com/mintytoast/tor-stuff/tree/bug_20716 :) *edit for correct branch!*

Last edited 3 years ago by icanhasaccount (previous) (diff)

comment:3 Changed 3 years ago by icanhasaccount

Status: newneeds_review

comment:4 in reply to:  2 Changed 3 years ago by cypherpunks

Replying to icanhasaccount:

I made an attempt at a fix: https://github.com/mintytoast/tor-stuff/tree/bug_19563 :)

This looks like the wrong branch, did you mean https://github.com/mintytoast/tor-stuff/tree/bug_20716 perhaps?

comment:5 Changed 3 years ago by icanhasaccount

Gah - yup, I've updated the link - thanks for the heads up:)

comment:6 Changed 3 years ago by arma

I think one question we need to answer is: which string would we like in the address field?

The first string we write is "AF_UNIX". The second string we write is "/home/arma/.tor/socks", i.e. the file system path for the sockssocket.

I think the second one is a little bit more useful, since if we have multiple sockssockets, then conn->address will tell us which one caused the stream?

comment:7 Changed 3 years ago by icanhasaccount

Heya - I went with current behavior in the patch.

I wonder if it would be worth initializing port to 0 in the declaration? (the comment in the connection_t struct in or.h suggests this would be okay).

The other thing is we aren't checking the return code for tor_addr_from_sockaddr - not sure if this is an issue though..

comment:8 in reply to:  7 Changed 3 years ago by arma

Replying to icanhasaccount:

The other thing is we aren't checking the return code for tor_addr_from_sockaddr - not sure if this is an issue though..

The function can only fail if the family isn't v4, v6, or unix. So it can't fail here.

I wonder if Nick would happily take a patch to clean up all the return codes? It seems awfully messy to have a bunch of error handling that we should never use though. I wonder if there's a way to refactor (our use of) the function so it's never expected to fail?

comment:9 Changed 3 years ago by nickm

Your patch is trying to test for the opposite of new_type == CONN_TYPE_AP && conn->socket_family == AF_UNIX -- but that isn't new_type != CONN_TYPE_AP && conn->socket_family != AF_UNIX. It's new_type != CONN_TYPE_AP || conn->socket_family != AF_UNIX.

(Remember, !(a && b) == !a || !b.)

How about what I have in bug20716_026 in my public repository?

https://gitweb.torproject.org/nickm/tor.git/commit/?h=bug20716_026&id=3b6da3f90ccad60517f5b639d5340a7e6489be27

comment:10 Changed 3 years ago by nickm

Keywords: 028-backport 027-backport 026-backport added

comment:11 Changed 3 years ago by icanhasaccount

Hi Nick

Ugh, I think I need to read through K+R again before I send any more patches ><

Your patch reads fine to me FWIW.

The other thing I'd considered was splitting up the (conn->socket_family == AF_UNIX && new_type == CONN_TYPE_AP) case into a separate conditional (this happens for (conn->socket_family == AF_UNIX && conn->type != CONN_TYPE_AP) below but I can always send in a patch for that later if its worth it.

Thanks again and for the feedback

comment:12 Changed 3 years ago by nickm

Keywords: review-group-13 added

comment:13 Changed 3 years ago by nickm

Reviewer: nickm

comment:14 Changed 3 years ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.2.8.x-final

Thanks! Merged in 0.2.9 and 0.3.0; marking as possible backport

comment:15 Changed 3 years ago by nickm

Keywords: review-group-13 removed

comment:16 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final
Resolution: fixed
Status: needs_reviewclosed

No backport to 0.2.8.

Note: See TracTickets for help on using tickets.