MySQL Bugs: #62452: NPE thrown in JDBC4MySQLPooledException when statement is closed

Bug #62452	NPE thrown in JDBC4MySQLPooledException when statement is closed
Submitted:	16 Sep 2011 16:34	Modified:	12 May 2015 21:36
Reporter:	James Roper	Email Updates:
Status:	Closed	Impact on me:	None
Category:	Connector / J	Severity:	S3 (Non-critical)
Version:	5.1.13, 5.1.17	OS:	Linux
Assigned to:	Alexander Soklakov	CPU Architecture:	Any
Tags:	Contribution

Description:
JDBC4MySQLPooledConnection synchronizes on its list of statementEventListeners when fireStatementEvent() is called.  However, its close() method sets that list to be null.  When a prepared statement is closed, fireStatementEvent() is called.  So, if code is correctly cleaning everything up when an exception occurs that results in the connection being closed, an NPE will be thrown.

How to repeat:
Example code:

Connection conn = null;
PreparedStatement stmt = null;
try {
  // (1) Obtain a connection, create a statement and do something
} finally {
  // Close everything to make sure we've cleaned up
  try {
    if (stmt != null) stmt.close(); // (2)
  } catch (SQLException e) {
    // Ignore
  }
  try {
    if (conn != null) conn.close();
  } catch (SQLException e) {
    // Ignore
  }
}

If a communication exception of some sort occurs during (1), the finally block will be executed, and at that point the connection will be already closed.  At this point, at (2), a NullPointerException is thrown because the closed connection has nulled out the list that it synchronises on when the closed even is fired.

Suggested fix:
Don't ever synchronize on a non final field, that's asking for trouble.

What exact version of Connector/J do you use?

I've updated the version with the version that I have reproduced this error on.  But I've also checked the source code latest version (5.1.17) and found that the bug is still there.  Specifically, in JDBC4PreparedStatementWrapper, on line 82 in the close() method, we see a statement event being fired on the connection:

  if (con instanceof JDBC4MysqlPooledConnection) {
    ((JDBC4MysqlPooledConnection) con).fireStatementEvent(e);

Then in JDBC4MysqlPooledConnection.fireStatementEvent(), we see a synchronize on the field statementEventListeners:

  void fireStatementEvent(StatementEvent event) throws SQLException {
    synchronized (this.statementEventListeners) {

but, if the connection is already closed, then it's closed method will have run, which sets statementEventListeners to null, hence the above can throw an NPE:

  public synchronized void close() throws SQLException {
    super.close();
    if (this.statementEventListeners != null) {
      this.statementEventListeners.clear();
      this.statementEventListeners = null;
    }
  }

Since the close() method is already synchronizing on this, my suggested fix is to synchronise every method that uses statementEventListeners on this, and either don't set statementEventListeners to null, or always null check before using it.

Patch of an example fix

Attachment: fix.patch (application/octet-stream, text), 1.27 KiB.

Bug#62445 & Bug#37645.

We have been experiencing this bug in production with Connector/J 5.1.17, Jackrabbit 1.6.4, and JBoss 4.2.3 on a sporadic basis.  It was a severe issue in that, once it started to occur, Jackrabbit could not create new connections to the database, which completely broke our application. We fixed the issue by applying the suggested patch to the 5.1.18 source code. After about a week in production, we see that, when Jackrabbit occasionally has a connection error, it successfully re-establishes the connection and the application carries on without users noticing anything.

Just as an FYI, I did a cursory review of the driver source code using a static code analyzer and found quite a few similar issues with synchronization in other classes. Also found a few obvious bugs such as a string comparison using "==" instead of .equals.

One problem with the synchronization flow analysis of a static code analyzer is it can't see the contract that's enforced (not by code) of the JDBC API. I've used Findbugs to point out obvious synchronization issues, it appears we have more to cover, but there are some that will jump out that are false positives, because making it correct in static analysis' eyes would mean the driver would deadlock when used in ways that are legal (and common) according to the JDBC API specification, while the access path in these cases by multiple threads is disallowed by the same specification.

I'll make sure the team is looking at findbugs more often to catch the simple issues you've pointed out.

Verified by code review.

Added the following entry to the Connector/J 5.1.36 changelog:

"JDBC4MySQLPooledConnection synchronizes its list of statementEventListener instances whenever the fireStatementEvent() method is called. However, because the pooled connection's close() method set the list to be null, if a prepared statement was closed after its holding pooled connection had already been closed, the subsequent fireStatementEvent() call would run into a null point exception. This fix prevents the problem by having JDBC4MySQLPooledConnection initializing the list of statement event listeners properly and never setting the list to null, thus allowing the list to be used all the time as a monitor lock."

Updated the changelog entry for the bug to the following:

"JDBC4MySQLPooledConnection keeps a list of statementEventListener instances named statementEventListeners, which is used as monitor lock whenever the fireStatementEvent() method is called. However, because the pooled connection's close() method set statementEventListener to be null, if a prepared statement was closed after its holding pooled connection had already been closed, the subsequent fireStatementEvent() call would run into a null point exception. This fix prevents the problem by having JDBC4MySQLPooledConnection initializing statementEventListeners properly and never setting it to null, thus allowing it to be used all the time as a monitor lock."