Opened 7 years ago

Closed 7 years ago

#2946 closed defect (fixed)

avoid handling DNSServer events after free

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20130528
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 4.72 Internal?: no

Description

See http://bind10.isc.org/ticket/2942#comment:18

This task is to give the problem a complete solution. We'll need to
do something like including a shared pointer of the server object
itself, e.g. using shared_from_this, in the callback parameter.

See an example at: http://think-async.com/asio/asio-1.4.8/doc/asio/tutorial/tutdaytime3.html#asio.tutorial.tutdaytime3.the_tcp_connection_class

We'll have to do this for all derived classes of DNSServer.

I also believe we can close #2556 when this task is completed.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 5

Estimated during the sprint planning call.

comment:2 Changed 7 years ago by muks

  • Milestone changed from Next-Sprint-Proposed to Sprint-20130528

comment:3 Changed 7 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:4 Changed 7 years ago by jinmei

trac2946 is ready for review.

the changes (except the last commit, see below) should be
straightforward with the understanding the problem.

regarding other servers than SyncUDPServer, on a closer lookup (+
some experiments) I realized they don't have this issue, at least in
an obvious way like the case of SyncUDPServer. Other server
implementations passes a copy of the server object, sharing actual
resources via member shared_ptr's.

I noticed one possible point of use-after-free in TCPServer, which
was fixed in the last commit. The change should be straightforward,
although I couldn't come up with a reasonable test scenario to prove
my theory. So, if this is considered a premature change I'm okay with
excluding it.

proposed changelog:

614.?	[bug]		jinmei
	b10-auth: Avoid referencing to a freed object when authoritative
	server addresses are reconfigured.  It caused a crash on a busy
	server during initial startup time, and the same crash could also
	happen if listen_on parameters are reconfigured run time.
	(Trac #2946, git TBD)

comment:5 Changed 7 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:6 Changed 7 years ago by jinmei

note: buildbots reported some build errors, so I updated the branch.
I believe it now builds on these platforms. Due to the additional
commit "the last commit" I mentioned in my previous ticket comment
is not last anymore. It's commit 935c7cc.

comment:7 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:8 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

I think it should be „reconfigured _at_ runtime“ in the changelog.

Also, in the added test, the this->stopIOService looks awkward to me, because signal takes function, not a method. I guess it's a static method, so should it be called by :: instead of ->?

Anyway, it seems OK, so please merge with or without changes to these.

comment:9 Changed 7 years ago by vorner

  • Total Hours changed from 0 to 0.52

comment:10 in reply to: ↑ 8 Changed 7 years ago by jinmei

Replying to vorner:

I think it should be „reconfigured _at_ runtime“ in the changelog.

Okay, added it in the actual changelog entry.

Also, in the added test, the this->stopIOService looks awkward to me, because signal takes function, not a method. I guess it's a static method, so should it be called by :: instead of ->?

On a closer look I realized we simply didn't need any prefix, this
or DNSServerTestBase as it's a public (static) member of a base
class. I changed it that way, and merged.

Now closing.

comment:11 Changed 7 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0.52 to 4.72
Note: See TracTickets for help on using tickets.