Details

    • Bug behavior facts:
      Security

      Description

      There are quite a few instances of public static fields which appear to be intended to be constant, but which are not final.

      The code would be safer if all the constant fields were made final.

      Patch to follow for some of the problem fields.

      The following also ought to be fixed:

      org.apache.derby.iapi.services.property.PropertyUtil.servicePropertyList (e.g. use an accessor to lookup the values)
      org.apache.derby.iapi.types.JSQLType.primitiveNames (ditto)

      1. derbyfinal.patch
        4 kB
        Sebb
      2. derbyfinal2.patch
        11 kB
        Sebb

        Activity

        Hide
        Knut Anders Hatlen added a comment -

        Thanks for the patch, Sebb! It looks fine to me. Committed revision 789313.

        Show
        Knut Anders Hatlen added a comment - Thanks for the patch, Sebb! It looks fine to me. Committed revision 789313.
        Hide
        Sebb added a comment -

        More public static fields patched.
        Note that it was necessary to make JSQLType.primitiveNames private and introduce a getter method to prevent write access to the array.

        Also, it was easier to make Configuration.dncProductVersionHolder__ private than to make it final as there was already a getter method.

        Show
        Sebb added a comment - More public static fields patched. Note that it was necessary to make JSQLType.primitiveNames private and introduce a getter method to prevent write access to the array. Also, it was easier to make Configuration.dncProductVersionHolder__ private than to make it final as there was already a getter method.
        Hide
        Sebb added a comment -

        The Configuration class stilll has lots of mutable static public fields, and needs quite a lot more work to ensure that static fields cannot be arbitrarily changed at run-time.

        Show
        Sebb added a comment - The Configuration class stilll has lots of mutable static public fields, and needs quite a lot more work to ensure that static fields cannot be arbitrarily changed at run-time.
        Hide
        Knut Anders Hatlen added a comment -

        derbyfinal2.patch looks like a good improvement to me. I'll run the regression tests and hopefully commit it later today.

        Some of the fields touched in the Configuration class don't actually seem to be in use, except in the static block that initializes them. Those are jreLevel, jreLevelMajor, jreLevelMinor, dncPackageVersion and packageNameForDNC. Do you think it would be best just to get rid of them altogether?

        As to the other problematic fields in Configuration, I think many of them are unused an can be removed. dncPackageConsistencyToken, which is an array, should probably be moved to NetConfiguration, in which case it could be made package-private (then it cannot be accessed by external code since the package is sealed). dncCompatibleJREVersions is another public array, but since it's only used for debugging/tracing purposes, there's probably not much harm that could be done by maliciously changing it. (The current values in dncCompatibleJREVersions are not correct, though, since the client driver no longer works with 1.3, and it does work with 1.5 and 1.6. But that's a different issue, I guess.)

        Show
        Knut Anders Hatlen added a comment - derbyfinal2.patch looks like a good improvement to me. I'll run the regression tests and hopefully commit it later today. Some of the fields touched in the Configuration class don't actually seem to be in use, except in the static block that initializes them. Those are jreLevel, jreLevelMajor, jreLevelMinor, dncPackageVersion and packageNameForDNC. Do you think it would be best just to get rid of them altogether? As to the other problematic fields in Configuration, I think many of them are unused an can be removed. dncPackageConsistencyToken, which is an array, should probably be moved to NetConfiguration, in which case it could be made package-private (then it cannot be accessed by external code since the package is sealed). dncCompatibleJREVersions is another public array, but since it's only used for debugging/tracing purposes, there's probably not much harm that could be done by maliciously changing it. (The current values in dncCompatibleJREVersions are not correct, though, since the client driver no longer works with 1.3, and it does work with 1.5 and 1.6. But that's a different issue, I guess.)
        Hide
        Knut Anders Hatlen added a comment -

        derbyfinal2.patch was committed to trunk with revision 789684.

        Show
        Knut Anders Hatlen added a comment - derbyfinal2.patch was committed to trunk with revision 789684.
        Hide
        Sebb added a comment -

        Sorry, I've no idea of whether the Configuration variables are needed or not.

        I just ran a PMD script that looks for mutable static variables and tried to make them final where possible. Defensive coding.

        Show
        Sebb added a comment - Sorry, I've no idea of whether the Configuration variables are needed or not. I just ran a PMD script that looks for mutable static variables and tried to make them final where possible. Defensive coding.
        Hide
        Kristian Waagan added a comment -

        The state of this issue isn't quite clear to me, but I cleared the patch available flag as both patches have been committed.

        Show
        Kristian Waagan added a comment - The state of this issue isn't quite clear to me, but I cleared the patch available flag as both patches have been committed.
        Hide
        Bryan Pendleton added a comment -

        Marked as resolved. I believe Knut committed both patches successfully.

        Show
        Bryan Pendleton added a comment - Marked as resolved. I believe Knut committed both patches successfully.

          People

          • Assignee:
            Knut Anders Hatlen
            Reporter:
            Sebb
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development