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

[BIND 10 Development]

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

 * hours:  5 => 6
 * status:  reviewing => closed
 * resolution:   => fixed
 * totalhours:  55 => 61


Comment:

 Replying to [comment:19 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.

 I have fixed the exception.
 >
 > 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.

 This is a good catch.  In fact, none of the Postgresql functions which
 free
 resources set the pointer to NULL.  I have added a set of conn_ to NULL.

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

 Fixed.

 > 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.

 There is one case in which you would need it, though it's rather esoteric.
 If
 upon entry the vector contained n elements and NUM_STATEMENTS is greater
 than n
 but the index of the tagged_statement element that is empty is less than
 n-1,
 you could end up with vestigial" elements.   Since this is not something
 called
 often, typically once at start-up, I'm going to leave it for now.

 The remainder of the issues will be addressed under 3382.


 Changes are merged in with git 1aae8b1fab3008e62c4f085948b1abadad512447
 Added ChangeLog entry 777 for it.

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