Opened 9 years ago

Closed 7 years ago

#459 closed defect (fixed)

Dangerously counter-intuitive isc::auth::Query interface

Reported by: vorner Owned by:
Priority: low Milestone: Sprint-20120619
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: 6 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The Query constructor takes several const something& parameters. This allows for a temporary value to be passed as the parameter. And it is quite common to use such interface in this kind of manner.

But the Query class stores const references and uses them later on. So, if the Query is use like this:

Query mx_query(memory_datasrc, Name("mx.example.com"), RRType::MX(), response);
mx_query.process();

then it does not do what is expected. It creates the Name. Then reference to it is passed to the Query constructor and stored. After the constructor is terminated, the Name is destroyed. So, when we call .process(), the reference already points to a place on the stack where the Name is no longer present, which seems quite dangerous. However, this code works like expected:

Name qname("mx.example.com");
Query mx_query(memory_datasrc, qname, RRType::MX(), response);
mx_query.process();

This behaviour is counter-intuitive and isn't even mentioned in the documentation. I think it should be changed (either use non-const reference, pointer or make a copy of the parameters). If there really is reason for such strange expectation about the parameters, it should be documented at last.

Subtickets

Change History (6)

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

Replying to vorner:

The Query constructor takes several const something& parameters. This allows for a temporary value to be passed as the parameter. And it is quite common to use such interface in this kind of manner.

But the Query class stores const references and uses them later on. So, if the Query is use like this:

Query mx_query(memory_datasrc, Name("mx.example.com"), RRType::MX(), response);
mx_query.process();

[...]

This behaviour is counter-intuitive and isn't even mentioned in the documentation. I think it should be changed (either use non-const reference, pointer or make a copy of the parameters). If there really is reason for such strange expectation about the parameters, it should be documented at last.

I admit the interface is slippery. The main reason for passing references is to avoid copy, and in the expected usage of query processing by b10-auth, the parameters should directly come from the question, which is expected to be valid until the query processing is completed.

But since this interface is half open I agree it should be safer. Some possibilities are:

  • make "Query" is just a function (at this moment there's not a strong reason it has to be a class)
  • likewise, we could merge the process() method in the Query constructor
  • pass pointers instead of references

comment:2 Changed 8 years ago by jinmei

  • Defect Severity set to N/A
  • Milestone set to Next-Sprint-Proposed
  • Sub-Project set to DNS

comment:3 Changed 8 years ago by jinmei

I guess the problem has gone in the latest interface (as a result of
the performance enhancement work). We should simply confirm it
and close the ticket.

comment:4 Changed 8 years ago by jelte

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

comment:5 Changed 8 years ago by jelte

  • Priority changed from medium to low

comment:6 Changed 7 years ago by vorner

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

Yes, looking at the code, the parameter is passed to process directly and it is not preserved outside the method. The problem is gone, so I'm closing it.

Note: See TracTickets for help on using tickets.