Derby
  1. Derby
  2. DERBY-5561

Race conditions in LogicalConnection checking for a null physical connection

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.5.3.2, 10.8.2.2
    • Component/s: Network Client
    • Labels:
      None
    • Environment:
      Solaris 10
      Glassfish V2.1.1
      ClientXADataSource connection pool
    • Urgency:
      Urgent
    • Issue & fix info:
      High Value Fix
    • Bug behavior facts:
      Crash, Seen in production

      Description

      There are race conditions with checkForNullPhysicalConnection calls in LogicalConnection. checkForNullPhysicalConnection is not synchronized and it checks for the member "phsyicalConnection" which can be cleared by "nullPhsyicalConnection" (which is synchronized) and "close" (which is synchronized) and "closeWithoutRecyclingToPool" (which is synchronized).

      This affects "nativeSQL", "getAutoCommit", "getTransactionIsolation", "getWarnings", "isReadOnly", "getCatalog", "getTypeMap", "createStatement", "prepareCall", "prepareStatement", "setHoldability", "getHoldability", "setSavePoint", "rollBack", "releaseSavePoint", "getSchema", "setSchema".

      All of these call "checkForNullPhysicalConnection" and then use the member "physicalConnection" after that call returns. Because these methods are not synchronized, between the time "checkForNullPhysicalConnectoin" returns and "physicalConnection" is used, the "physicalConnection" member could be set to null and then a NPE occurs.

      Probably all of these methods should be changed to synchronized.

      1. DERBY-5561.patch
        7 kB
        Brett Bergquist
      2. derby-5561-2a-minor_cleanups.diff
        4 kB
        Kristian Waagan

        Activity

        Hide
        Kathey Marsden added a comment -

        Do you have multiple threads accessing the same connection at the same time? If so is it intentional? Typically when that is being done it is not intentional as even if the various methods were synchronized the two threads would share transaction and other state that would be difficult to coordinate.

        Show
        Kathey Marsden added a comment - Do you have multiple threads accessing the same connection at the same time? If so is it intentional? Typically when that is being done it is not intentional as even if the various methods were synchronized the two threads would share transaction and other state that would be difficult to coordinate.
        Hide
        Kathey Marsden added a comment -

        Makes sense. Thanks for the explanation.

        Show
        Kathey Marsden added a comment - Makes sense. Thanks for the explanation.
        Hide
        Kathey Marsden added a comment -
        Show
        Kathey Marsden added a comment - Just realized Brett's explanation is not in the Jira. It can be found in this thread. http://old.nabble.com/-jira---Created--%28DERBY-5561%29-Race-conditions-in-LogicalConnection-checking-for-a-null-physical-connection-to33046047.html#a33046825
        Hide
        Brett Bergquist added a comment -

        This patch changes all methods that call "checkForNullPhysicalConnection" to be synchronized. Some of the methods in LogicalConnection.java already were. This patch makes all calls consistent and eliminates the race condition.

        Show
        Brett Bergquist added a comment - This patch changes all methods that call "checkForNullPhysicalConnection" to be synchronized. Some of the methods in LogicalConnection.java already were. This patch makes all calls consistent and eliminates the race condition.
        Hide
        Siddharth Srivastava added a comment -

        thanks Brett for the patch. I will be looking into adding test for this patch.

        Show
        Siddharth Srivastava added a comment - thanks Brett for the patch. I will be looking into adding test for this patch.
        Hide
        Kathey Marsden added a comment -

        Triage for 10.9. Urgent, High Value Fix. Thanks Brett for the patch and Siddharth for picking up the testing and driving it to commit.

        Show
        Kathey Marsden added a comment - Triage for 10.9. Urgent, High Value Fix. Thanks Brett for the patch and Siddharth for picking up the testing and driving it to commit.
        Hide
        Kristian Waagan added a comment -

        Any updates on this issue?
        To me the changes look safe and correct. Given that the Connection interface probably won't see much change, and that writing a test that reliably detects missing synchronization in future methods (i.e. by using reflection and running a test pattern for each method) may be non-trivial, I suggest we commit the patch right away unless Siddharth is planning to write a test for this shortly.

        Siddhart, what's your opinion on this?

        Show
        Kristian Waagan added a comment - Any updates on this issue? To me the changes look safe and correct. Given that the Connection interface probably won't see much change, and that writing a test that reliably detects missing synchronization in future methods (i.e. by using reflection and running a test pattern for each method) may be non-trivial, I suggest we commit the patch right away unless Siddharth is planning to write a test for this shortly. Siddhart, what's your opinion on this?
        Hide
        Siddharth Srivastava added a comment -

        The patch works fine for me as well. I do plan to write the test for it but I can continue with it only after next week since I have exams till then.I'll add the test case for it after that.
        Until then I too recommend committing the patch.

        Show
        Siddharth Srivastava added a comment - The patch works fine for me as well. I do plan to write the test for it but I can continue with it only after next week since I have exams till then.I'll add the test case for it after that. Until then I too recommend committing the patch.
        Hide
        Kristian Waagan added a comment -

        Great!

        I committed DERBY-5561.patch to trunk with revision 1333305.

        Show
        Kristian Waagan added a comment - Great! I committed DERBY-5561 .patch to trunk with revision 1333305.
        Hide
        Kristian Waagan added a comment -

        Attaching patch 2a, which cleans up a few minor issues:
        o converts docs to Javadoc
        o remove stale doc
        o correct/improve some comments
        o makes physicalConnection_ package-private
        o makes checkForNullPhysicalConnection final

        Patch ready for review.

        Show
        Kristian Waagan added a comment - Attaching patch 2a, which cleans up a few minor issues: o converts docs to Javadoc o remove stale doc o correct/improve some comments o makes physicalConnection_ package-private o makes checkForNullPhysicalConnection final Patch ready for review.
        Hide
        Kristian Waagan added a comment -

        I committed patch 2a to trunk with revision 1334919.
        I'll give it a week or so before I look at backporting this to 10.8.

        Show
        Kristian Waagan added a comment - I committed patch 2a to trunk with revision 1334919. I'll give it a week or so before I look at backporting this to 10.8.
        Hide
        Kristian Waagan added a comment -

        Backported to 10.8 with revision 1344183.
        Resolving issue.

        Show
        Kristian Waagan added a comment - Backported to 10.8 with revision 1344183. Resolving issue.
        Hide
        Kathey Marsden added a comment -

        Likely applies to 10.5 as well so might be good to backport.

        Show
        Kathey Marsden added a comment - Likely applies to 10.5 as well so might be good to backport.
        Hide
        Kristian Waagan added a comment -

        Reopening for backports.

        Show
        Kristian Waagan added a comment - Reopening for backports.
        Hide
        Kristian Waagan added a comment -

        Backported to 10.7 with revision 1383996.
        I'll let the nightly tests run through before merging this fix back to 10.5, although I don't expect any problems.

        Show
        Kristian Waagan added a comment - Backported to 10.7 with revision 1383996. I'll let the nightly tests run through before merging this fix back to 10.5, although I don't expect any problems.
        Hide
        Mamta A. Satoor added a comment -

        Assigning to me for backporting to 10.5

        Show
        Mamta A. Satoor added a comment - Assigning to me for backporting to 10.5
        Hide
        Mamta A. Satoor added a comment -

        Backported to 10.6.2.3 with revision 1388139

        Show
        Mamta A. Satoor added a comment - Backported to 10.6.2.3 with revision 1388139
        Hide
        Mamta A. Satoor added a comment -

        Backported to 10.5.3.2 with revision 1388285

        Show
        Mamta A. Satoor added a comment - Backported to 10.5.3.2 with revision 1388285
        Hide
        Kristian Waagan added a comment -

        Resolving again. Added 10.5.3.2 to fix versions.

        Show
        Kristian Waagan added a comment - Resolving again. Added 10.5.3.2 to fix versions.
        Hide
        Knut Anders Hatlen added a comment -

        [bulk update] Close all resolved issues that haven't been updated for more than one year.

        Show
        Knut Anders Hatlen added a comment - [bulk update] Close all resolved issues that haven't been updated for more than one year.

          People

          • Assignee:
            Kristian Waagan
            Reporter:
            Brett Bergquist
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development