BIND 10 #3080: New DB backend: Postgres (patch)

[BIND 10 Development]

#3080: New DB backend: Postgres (patch)
-------------------------------------+-------------------------------------
            Reporter:  dclink        |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcpdb        |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  3             |                 CVSS Scoring:
         Total Hours:  55            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:  2592
                                     |          Add Hours to Ticket:  5
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by marcin):

 I went over the code in commit 8e7f2f910d6c7901706e60656b9107177470e725

 Several things listed below are nits. Several other things are more
 important and indicate that operations we perform here are unsafe (e.g.
 memcpy). In most cases they will not show up I guess. In case of memcpy,
 unless someone messes up in the lease database, the length of the data
 will remain within the boundary of the allocated buffer.

 Also, I don't think that our server's code will allow passing NULL leases
 to functions like addLease etc, therefore the NULL pointer assert may not
 show up.

 But, I insist that we rework the whole backend as soon as possible after
 the BIND10 1.2 release and treat this as urgent.

 The comments below will be referenced in #3382 as we probably want to
 address most of these issues there.

 '''src/lib/dhcpsrv/pgsql_lease_mgr.cc'''
 Functions in PgSqlLease4Exchange and PgSqlLease6Exchange should be
 documented.
 Also, the inner parts of the functions deserve some commentary too.

 createBindForSend doesn't check the lease object for being NULL. Note,
 that this function is directly called by the public functions of the lease
 manager. When someone passes a NULL pointer to these functions they will
 result in an assert which gets logged but is completely useless because
 there is no information where the assert origins.

 The exception thrown at line 282 is fubared. There is an opening
 paranthesis after !''longer!'' but not closing one. The maximal value is
 not in paranthesis at all.

 Nit: I tend to agree with Stephen that we should rather avoid the names
 for variables like !''tmp!''. This simply doesn't mean anything, except
 that it is a temporary variable (as almost all variables in the function
 scope) :)

 I guess it was out of scope to turn istringstream to lexical_cast?


 I wonder what is the benefit of initializing !''params!'' as a member of
 the !PgSqlLease4Exchange? There may be a benefit if params is returned as
 const reference, so as you don't have to copy construct the returned
 value. But, params are returned by value. Maybe they should be returned by
 const reference?

 convertFromDatabase: so pointer to !''r!'' is passed by reference? Any
 need for this?
 {{{
     convertFromDatabase(PGresult *& r, int line) {
 }}}

 I wonder if this is really safe operation:
 {{{
         memcpy(hwaddr_buffer_, hwaddr_str, hwaddr_length_);
         memcpy(client_id_buffer_, client_id_str, client_id_length_);
 }}}

 The hwaddr_length_ and client_id_length_ are set by the PQunescapeBytea
 which have undefined length? If they happen to be longer than the
 !HWAddr::MAX_HWADDR_LEN and !ClientId::MAX_CLIENT_ID_LEN they the memcpy
 will overrun the allocated array and cause undefined behavior.

 PgSqlLease4Exchange: hostname_ and hwaddr_ are unused.

 PgSqlLease6Exchange: duid_ is unused.

 convertFromDatabase: Perhaps instead of the switch construct we could just
 use cast:
 {{{
         if (lease_type > Lease6::TYPE_PD) {
             isc_throw(BadValue, "invalid lease type returned (" <<
                       lease_type_ << ") for lease with address " <<
                       addr6_ << ". Only 0, 1, or 2 are allowed.");
         }
         Lease6::Type type = static_cast<Lease6::Type>(lease_type);

 }}}

 openDatabase:
 {{{
     if (PQstatus(conn_) != CONNECTION_OK) {
         // If we have a connection object, we have to call finish
         // to release it, but grab the error message first.
         std::string error_message = PQerrorMessage(conn_);
         PQfinish(conn_);
         isc_throw(DbOpenError, error_message);
     }

 }}}

 AIUI, when database fails to open, the PQfinish will release the memory
 for conn_, but are we sure that the conn_ will be set to NULL by PQfinish?
 My guess is that it is not guaranteed, which in turn implies that the
 destructor may not invoke this:
 {{{
     if (conn_) {
         // Deallocate the prepared queries.
         PGresult* r = PQexec(conn_, "DEALLOCATE all");
         if(PQresultStatus(r) != PGRES_COMMAND_OK) {
             // Highly unlikely but we'll log it and go on.
             LOG_ERROR(dhcpsrv_logger, DHCPSRV_PGSQL_DEALLOC_ERROR)
                       .arg(PQerrorMessage(conn_));
         }

         PQclear(r);
         PQfinish(conn_);
     }
 }}}

 I am not sure whether it is an issue or not, but I woiuld set the conn_ to
 NULL to be sure.

 PgSqlLeaseMgr constructor: conn_, exchange4_ and exchange6_ could be
 initialized using the constructor's initialization list, rather than be
 initialized in the constructor's body.

 prepareStatements: There is no reason to call clear() before resize(). The
 resize() and then bunch of assignments should effectively remove and
 destroy objects present in the container.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3080#comment:19>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development