Opened 9 years ago

Closed 9 years ago

#394 closed defect (fixed)

memory leaks in branches/trac327 (after io_service refactors)

Reported by: jelte Owned by: jelte
Priority: medium Milestone:
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The auth and recurse servers both leak memory on each query (recurse much more than auth)

Subtickets

Change History (9)

comment:1 follow-up: Changed 9 years ago by jelte

I've fixed a few other memory-related problems in r3369, but these were not the big ones.

I've identified at least three places where new is used without a matching delete, but I'm not sure where to delete these (and/or if we can reach them at that point right now):

  • When RecursiveQuery::sendQuery() creates a UDPQuery object (asiolink.cc:240) , the constructor clones the DNSServer (udpdns.cc:180). This clone is never deleted.
  • udpdns.cc also does two news at line 109 and 110 which aren't freed

comment:2 in reply to: ↑ 1 Changed 9 years ago by jinmei

Replying to jelte:

I've fixed a few other memory-related problems in r3369, but these were not the big ones.

I've identified at least three places where new is used without a matching delete, but I'm not sure where to delete these (and/or if we can reach them at that point right now):

  • When RecursiveQuery::sendQuery() creates a UDPQuery object (asiolink.cc:240) , the constructor clones the DNSServer (udpdns.cc:180). This clone is never deleted.
  • udpdns.cc also does two news at line 109 and 110 which aren't freed

From a quick look, memory for "peer" and "iosock" seems to leak due to the following:

        peer = new TCPEndpoint(socket_->remote_endpoint());
        iosock = new TCPSocket(*socket_);
        io_message_.reset(new IOMessage(data_.get(), length, *iosock, *peer));

(in src/lib/asiolink/tcpdns.cc)

(for that matter this code is not exception safe, either)

comment:3 follow-up: Changed 9 years ago by each

The peer/iosock issue is because of the code being left in an intermediate state. Originally, peer and iosock were both shared_ptr<>'s and both members of the TCPServer and UDPServer classes, so they were deleted when the server instance was deleted. I moved them into the operator() function while I was experimenting with changing the way IOMessage is constructed, ran into a roadblock, and reverted most of the change, but apparently I left this bit wrong.

Anyway, this ought to fix it:

peer.reset(new TCPEndpoint(socket_->remote_endpoint()));
iosock.reset(new TCPSocket(*socket_));

My guess is you can fix the UDPQuery leak by calling "delete server_;" in a ~UDPQuery() destructor, oops.

comment:4 in reply to: ↑ 3 Changed 9 years ago by jelte

Replying to each:

The peer/iosock issue is because of the code being left in an intermediate state. Originally, peer and iosock were both shared_ptr<>'s and both members of the TCPServer and UDPServer classes, so they were deleted when the server instance was deleted. I moved them into the operator() function while I was experimenting with changing the way IOMessage is constructed, ran into a roadblock, and reverted most of the change, but apparently I left this bit wrong.

Anyway, this ought to fix it:

peer.reset(new TCPEndpoint(socket_->remote_endpoint()));
iosock.reset(new TCPSocket(*socket_));

ah, yes it does, r3379 (that one has one other change, i missed the shared_ptr vs shared_array in the tcp version, fixed that too).

we probably need to do something similar for the newed lenbuf at line tcpdns.cc:165

My guess is you can fix the UDPQuery leak by calling "delete server_;" in a ~UDPQuery() destructor, oops.

No, that won't work for the same reason it can't be a shared_ptr, UDPQuery falls out of scope...

comment:5 Changed 9 years ago by jelte

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Right,

I now think the comment above the server_ member was incorrect, and we can in fact use a shared pointer there (as long as clone() is used to fill it upon creation).

So between r3368 and r3419, I fixed all the memory leaks I could find in the auth and recurse modules. Ready for review

comment:6 Changed 9 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 Changed 9 years ago by vorner

  • Owner changed from vorner to jelte

The changes look ok and can be merged. There's just one small thing I noticed, but it does not seem really important: In tcpdns.cc, there's the lenbuf, which is created each time the coroutine is entered/reentered. It is small, but it could be moved into a block at the place where it is needed (most instances will be unused). I'll leave it up to you to decide if you want to do it or leave it as it is.

Valgrind still finds something it calls „possibly lost“ (which usually means probably not lost). I'd like to have a look at them at some point, but I think it is better to merge and close this now than keeping the branch old, as the possible leaks look like not related to -recurse or coroutines at all.

comment:8 Changed 9 years ago by vorner

I merged the branch to 327, I hope you don't mind. I leave the branch there and the ticket open for now just in case.

comment:9 Changed 9 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

no great, looks ok, closing ticket.

Note: See TracTickets for help on using tickets.