Opened 8 years ago

Closed 8 years ago

#1783 closed defect (fixed)

auth could cause duplicate buffer free on exception

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

Description

While looking into a regression due to #1600
(http://bind10.isc.org/ticket/1600#comment:36),
I noticed the deferred task of providing stronger exception guarantee
was more serious - in fact it would be a more fundamental exception
safety issue: if toWire() fails in processNotify() with an exception,
the renderer holds the buffer until the next query, but in the case of
TCP it might have been already deleted by the DNS server object.
So it could lead to a duplicate free (that should be happening in the
regression, btw).

So I'm going to fix the issue right now.

Subtickets

Change History (9)

comment:1 Changed 8 years ago by jinmei

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

comment:2 Changed 8 years ago by jinmei

trac1783 is ready for review. I believe this is easy to review.

I don't think we need a changelog for this.

comment:3 Changed 8 years ago by jinmei

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

comment:4 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:5 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to jinmei

Looks good.

Very similar to the QueryCleaner? we recently added :) Is there a known pattern name for this? (I.E. RAII-style resetting of reusable objects, RAII itself doesn't seem to apply).

Perhaps we should make a naming convention for it; holder/cleaner/whatever. Would be a bit of a bikeshed discussion in itself but it would be nice to be able to immediately see the purpose of such a class when it is used.

Anyway, it can be merged if not :)

comment:6 Changed 8 years ago by jelte

And when I pressed submit, I did think of one possible other change; perhaps the class should be public and defined with the messagerenderer itself. But no strong opinion on it.

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

Replying to jelte:

Thanks for the review.

Very similar to the QueryCleaner? we recently added :) Is there a known pattern name for this? (I.E. RAII-style resetting of reusable objects, RAII itself doesn't seem to apply).

I don't know of any common idiom to express this type of concept, but
there may be one because the technique itself is pretty commonly used
in C++. boost names some of such classes with "scoped" (scoped_ptr,
scoped_array, maybe some more), and I sometimes borrow that naming,
but it doesn't seem to be the "standard" convention.

Perhaps we should make a naming convention for it; holder/cleaner/whatever. Would be a bit of a bikeshed discussion in itself but it would be nice to be able to immediately see the purpose of such a class when it is used.

That sounds helpful. I'm not sure if we end up having a single common
naming convention (for example, "scope(d)" sometimes sounds natural in
that context but is probably not always so), but at least it would
help if we have a commonly used name for the concept (e.g. "scope
class").

And, about this point:

And when I pressed submit, I did think of one possible other change; perhaps the class should be public and defined with the messagerenderer itself. But no strong opinion on it.

Perhaps (at least I can see the need for it in b10-resolver), although
I personally think it would still be "project private" (so hidden in
somewhere under "internal" namespace, and not an install target)
rather than a real public definition. For now, this is the only user
of this helper so I keep it within the .cc file, but I added a note
that we may extend it and make it more visible to other modules.

I'm now going to merge this branch.

comment:8 Changed 8 years ago by jinmei

merge done, closing. I'm giving it an estimation point of 4.

comment:9 Changed 8 years ago by jinmei

  • Estimated Difficulty changed from 0 to 4
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 2.87
Note: See TracTickets for help on using tickets.