Opened 8 years ago

Closed 6 years ago

#2185 closed enhancement (fixed)

RR type implementation: TLSA

Reported by: shane Owned by: muks
Priority: high Milestone: bind10-1.2-release-freeze
Component: libdns++ 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: 0 Internal?: no

Description

Implement the TLSA type, from RFC 6698.

Subtickets

Change History (15)

comment:1 Changed 6 years ago by muks

  • Milestone set to Sprint-20131015
  • Status changed from new to reviewing

Up for review.

Note that empty certificate association data is allowed (as it's not clear if it's allowed or not, so we take the permissive side).

comment:2 Changed 6 years ago by muks

ChangeLog entry:

XYZ.    [func]          muks
        Add support for the TLSA RR type (RFC 6698).
        (Trac #2185, git ...)

comment:3 Changed 6 years ago by shane

Ticket #3298 depends on this being complete.

comment:4 Changed 6 years ago by shane

  • Priority changed from medium to high

comment:5 follow-up: Changed 6 years ago by pselkirk

  • Owner changed from UnAssigned to muks

On a quick read through the RFC, I don't see where it is implied that the Certificate Association Data field could be empty, or how it might be useful or meaningful.

That aside, I find nothing to object to here, except to note that auto_ptr is officially deprecated as of C++11, but we have a number of other Rdata classes that use it in exactly the same way.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 6 years ago by muks

Hi Paul

Replying to pselkirk:

On a quick read through the RFC, I don't see where it is implied that the Certificate Association Data field could be empty, or how it might be useful or meaningful.

This is mentioned in comment:1. It is not implied both ways, so we take a permissive stance. We usually are not strict about the range of values an RDATA field can take. If we have a single-octet field for example and only values 1 and 2 are currently specified (used), we allow all values usually.

I don't know how empty data could be useful for particular values of other RDATA fields. But this is a valid point. I'll ask about this and reply on this ticket.

That aside, I find nothing to object to here, except to note that auto_ptr is officially deprecated as of C++11, but we have a number of other Rdata classes that use it in exactly the same way.

Nod. Deprecation of auto_ptr was mentioned in the "High Integrity C++" document that Shane shared sometime back (http://www.codingstandard.com/). But note that it's not possible to use shared_ptr in the RDATA class itself for managing the pimpl as it is opaque in that public scope (also mentioned in the ticket linked from comment:2). We also cannot currently use any C++11 specific features.

Do you have any suggestions to replace auto_ptr (short of explicit memory management using delete)? See #3298.

comment:7 in reply to: ↑ 6 Changed 6 years ago by muks

Replying to muks:

Do you have any suggestions to replace auto_ptr (short of explicit memory management using delete)? See #3298.

I guess instead of auto_ptr, we can use a templated RdataPimplHolder class (similar to SegmentObjectHolder and other such similar classes in the tree) which implement the RAII pattern with a .release() method. Would this be an acceptable choice? If so, I'll create a ticket for it.

comment:8 Changed 6 years ago by muks

I have introduced RdataPimplHolder in trac2185 that is a drop-in replacement for std::auto_ptr in the way it is used in the various RDATA parsers. I've created ticket #3334 to do this for all RR types after this branch has been merged to master.

comment:9 Changed 6 years ago by muks

  • Owner changed from muks to UnAssigned

This is ready for re-review. All review comments have been addressed.

comment:10 follow-up: Changed 6 years ago by pselkirk

  • Owner changed from UnAssigned to muks

My intent wasn't to cause more work, just to point out that auto_ptr has been deprecated, so this code might not work with some future version of the compiler.

The new RdataPimplHolder class is an interesting experiment, and probably not heavier-weight than auto_ptr, but it's more code to test and maintain (and to update the 7 other Rdata classes that use auto_ptr), so I would tend to resist it on those grounds.

Also, it doesn't compile:

In file included from rdata_pimpl_holder_unittest.cc:15:0:
../../../../src/lib/dns/rdata_pimpl_holder.h:27:31: error: ‘NULL’ was not declared in this scope
     RdataPimplHolder(T* obj = NULL) :
                               ^
../../../../src/lib/dns/rdata_pimpl_holder.h:35:25: error: ‘NULL’ was not declared in this scope
     void reset(T* obj = NULL) {
                         ^
../../../../src/lib/dns/rdata_pimpl_holder.h: In member function ‘T* isc::dns::rdata::RdataPimplHolder<T>::release()’:
../../../../src/lib/dns/rdata_pimpl_holder.h:46:16: error: ‘NULL’ was not declared in this scope
         obj_ = NULL;
                ^
rdata_pimpl_holder_unittest.cc: In member function ‘virtual void {anonymous}::RdataPimplHolderTest_all_Test::TestBody()’:
rdata_pimpl_holder_unittest.cc:39:19: error: call to ‘void isc::dns::rdata::RdataPimplHolder<T>::reset(T*) [with T = int]’ uses the default argument for parameter 1, which is not yet defined
     holder2.reset();

In commit 506b531, I see you now disallow empty Certificate Association Data, which addresses my other comment in comment:5.

comment:11 in reply to: ↑ 10 Changed 6 years ago by muks

  • Owner changed from muks to pselkirk

Hi Paul

Replying to pselkirk:

My intent wasn't to cause more work, just to point out that auto_ptr has been deprecated, so this code might not work with some future version of the compiler.

Nod. We have generally avoided using std::auto_ptr except where absolutely necessary, using boost::scoped_ptr and boost::shared_ptr instead as applicable.

The new RdataPimplHolder class is an interesting experiment, and probably not heavier-weight than auto_ptr, but it's more code to test and maintain (and to update the 7 other Rdata classes that use auto_ptr), so I would tend to resist it on those grounds.

The RdataPimplHolder is used as drop-in replacement for std::auto_ptr, so it won't be difficult to understand it. The code and tests are written, and for something this straightforward and independent, it is unlikely any maintenance will be necessary.

This whole exercise is because std::auto_ptr is deprecated. We should avoid using it in new code, and it's unlikely we can rely on C++11-only features like std::unique_ptr:

The RDATA classes are a major user of std::auto_ptr. The rest of the classes also seem to use it similar to how the RDATA classes use it. In any other cases that do not require .release(), they use boost::scoped_ptr and boost::shared_ptr.

I suggest even renaming this class and moving it to libb10-util as a general smart pointer.

But if you have a strong opinion that it should not go in, I'll revert it and switch back to std::auto_ptr.

Also, it doesn't compile:

In file included from rdata_pimpl_holder_unittest.cc:15:0:
../../../../src/lib/dns/rdata_pimpl_holder.h:27:31: error: ‘NULL’ was not declared in this scope
     RdataPimplHolder(T* obj = NULL) :
                               ^
../../../../src/lib/dns/rdata_pimpl_holder.h:35:25: error: ‘NULL’ was not declared in this scope
     void reset(T* obj = NULL) {
                         ^
../../../../src/lib/dns/rdata_pimpl_holder.h: In member function ‘T* isc::dns::rdata::RdataPimplHolder<T>::release()’:
../../../../src/lib/dns/rdata_pimpl_holder.h:46:16: error: ‘NULL’ was not declared in this scope
         obj_ = NULL;
                ^
rdata_pimpl_holder_unittest.cc: In member function ‘virtual void {anonymous}::RdataPimplHolderTest_all_Test::TestBody()’:
rdata_pimpl_holder_unittest.cc:39:19: error: call to ‘void isc::dns::rdata::RdataPimplHolder<T>::reset(T*) [with T = int]’ uses the default argument for parameter 1, which is not yet defined
     holder2.reset();

It did not fail compile for me on Fedora 20 with GCC. I am guessing it is failing in a different build environment (perhaps with a different standard C++ library).

I have now explicitly included <cstddef> that provides NULL's definition.

comment:12 Changed 6 years ago by stephen

  • Owner changed from pselkirk to muks

Reviewed commit 569e7f0737f69d0c2768bdc02f0b0961e992a24c (in particular, the changes between it and the commit last reviewed by Paul, 506b5310b54aa834f2a1dcdff167f91a07b974fa).

All changes OK, please merge.

comment:13 Changed 6 years ago by muks

  • Owner changed from muks to stephen

Hi Stephen

Please can you review commit 080f7d6b93825785ebdca4aaa64df2f636d4e54a on the branch? I had to merge from master and add this extra commit as master had moved on since the branch point and has removed API.

comment:14 Changed 6 years ago by stephen

  • Owner changed from stephen to muks

Please can you review commit 080f7d6b93825785ebdca4aaa64df2f636d4e54a on the branch? I had to merge from master and add this extra commit as master had moved on since the branch point and has removed API.

Reviewed. All OK, please merge.

comment:15 Changed 6 years ago by muks

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

Merged to master branch in commit a168170430f6927f28597b2a6debebe31cf39b13:

* 080f7d6 [2185] Stop using API that was removed
* 569e7f0 [2185] Add include for NULL definition
* 506b531 [2185] Don't allow empty TLSA certificate association data
* 5813a6d [2185] Add tests for RdataPimplHolder
* 9ac39be [2185] Add and use a RdataPimplHolder instead of auto_ptr
* d88bd04 [2185] Add support for the TLSA RR type

Resolving as fixed. Thank you for the reviews Stephen.

Note: See TracTickets for help on using tickets.