Opened 7 years ago

Closed 7 years ago

#2821 closed defect (fixed)

MySQLLeaseManager does not close db if constructor fails

Reported by: jelte Owned by: jelte
Priority: medium Milestone: Sprint-DHCP-20130328
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This looks like it is currently the last issue that makes the valgrind bot fail, and I would like to fix this before other issues crop up again (so that we can finally address new valgrind issues in the tickets that cause them).

The MySQLLeaseMgr class holds a pointer to an open database connection, which is closed when an instance is destroyed. However, during initialization several internal calls are made and if any of those (and hence construction) fails the database is not closed.

I have a fix, which is partly the Right Approach but also partly path of least resistance (it introduces an RAII-style object but only uses it in the constructor for now, and not in any calls it makes or the main class itself), because I did not want to touch too much code.

I'll push it to a branch as soon as trac gives me a ticket number.

Subtickets

Change History (10)

comment:1 Changed 7 years ago by jelte

  • Owner set to UnAssigned
  • Status changed from new to reviewing

pushed, see commit in trac2821

comment:2 Changed 7 years ago by shane

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:3 Changed 7 years ago by stephen

  • Milestone changed from Next-Sprint-Proposed to Sprint-DHCP-20130228

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 Changed 7 years ago by stephen

  • Owner changed from stephen to jelte

Reviewed commit 21dae0aa029e8d549c8ba4d5bffeb10ef341f17d

I'm not happy with the "release" mechanism - it's somewhat inelegant. On reflection, I think agree with the comment in the commit, that it would be better to completely encapsulate the database handle in an RAII class (perhaps a subclass of MySqlLeaseMgr) along the lines of:

class MySqlHolder {
public:
     MySqlHolder() : mysql_(mysql_init(NULL)) {
         if (mysql_ == NULL) {
             isc_throw(DbOpenError, "unable to initialize MySQL");
         }
     }


    ~MySqlHolder() {
        if (mysql_ != NULL) {
            mysql_close(mysql_);
        }
    }

    operator MYSQL*() const {
        return mysql_;
    }

private:
    MYSQL* mysql_;
};

I don't think the changes needed would be extensive. Could we please implement this?

comment:6 Changed 7 years ago by jelte

  • Owner changed from jelte to stephen

how's this?

(took class from your comment, and updated the constructor).

As said on jabber, I also added a mysql_library_end() call to the main unit tests (if mysql is available), becuase that reported a whole bunch of additional unfreed data within mysql. Weird that that didn't pop up earlier, tbh, maybe different versions of mysql handle that better...

comment:7 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to jelte

Reviewed commit e58c2fd32cb85d37cd96ff8bb6a7cdf0f653a5a1

I've added a set of comments to the MySqlHolder class and pushed the changes.

One minor thing: is there any reason that the call to mysql_library_end() can't go in the MySqlHolder destructor? mysql_lease_mgr.* are the only files that refer to MySql, so it seems logical that all MySql-related stuff should go there.

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

  • Owner changed from jelte to stephen

Replying to stephen:

I've added a set of comments to the MySqlHolder class and pushed the changes.

thanks!

One minor thing: is there any reason that the call to mysql_library_end() can't go in the MySqlHolder destructor? mysql_lease_mgr.* are the only files that refer to MySql, so it seems logical that all MySql-related stuff should go there.

ok, done.

updated the create and destroy code in the unit tests to use the holder as well

comment:9 Changed 7 years ago by stephen

  • Owner changed from stephen to jelte

Reviewed commit 17c0037b8ab08e7aaa1a9eee9e1a0753308b2ce2

The use of the holder class has certainly simplifies the use of MySql in the {create,destroy}Schema methods in unit tests.

Looks good, it can be merged.

comment:10 Changed 7 years ago by jelte

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

thanks! merged, closing ticket.

Note: See TracTickets for help on using tickets.