Derby
  1. Derby
  2. DERBY-5357

SQLJ.INSTALL_JAR shouldn't use identifier as file name

    Details

    • Urgency:
      Normal
    • Bug behavior facts:
      Security

      Description

      When installing a jar file with the SQLJ.INSTALL_JAR procedure, it will copy the jar file to a subdirectory of the database directory. The name of the stored jar file is based on the qualified name specified by the second parameter in the procedure, and becomes something like: <DBDIR>/jar/<SCHEMA>/<JAR_NAME>.jar.<VERSION>

      This naming scheme is problematic because the qualified name of the jar file is an SQL identifier and may contain any characters, also characters with special meaning to the underlying file system.

      One example is this call:

      ij> call sqlj.install_jar('/path/to/toursdb.jar', 'APP."../../../x/jar"', 0);
      0 rows inserted/updated/deleted

      On Unix-like systems, this will install the jar in a subdirectory of the database directory's parent directory, which is clearly unfortunate as the database directory should be self-contained (an assumption used when taking backup of a database using operating system commands, or when moving the database to another location).

      There's probably also a possibility that INSTALL_JAR fails if the identifier contains a character that's not allowed in file names on the platform.

      It would be better if the jars were stored in a file whose name is independent of the identifier used, so that any valid SQL identifier could be used to name a jar file in the database without causing problems.

      1. derby-5357-with-tests-4.stat
        0.8 kB
        Dag H. Wanvik
      2. derby-5357-with-tests-4.diff
        46 kB
        Dag H. Wanvik
      3. derby-5357-with-tests-3.stat
        0.8 kB
        Dag H. Wanvik
      4. derby-5357-with-tests-3.diff
        39 kB
        Dag H. Wanvik
      5. derby-5357-with-tests-2.stat
        0.8 kB
        Dag H. Wanvik
      6. derby-5357-with-tests-2.diff
        38 kB
        Dag H. Wanvik
      7. derby-5357-with-tests.stat
        0.8 kB
        Dag H. Wanvik
      8. derby-5357-with-tests.diff
        36 kB
        Dag H. Wanvik
      9. derby-5357-2.stat
        0.6 kB
        Dag H. Wanvik
      10. derby-5357-2.diff
        22 kB
        Dag H. Wanvik
      11. derby-5357.stat
        0.1 kB
        Dag H. Wanvik
      12. derby-5357.diff
        2 kB
        Dag H. Wanvik

        Activity

        Knut Anders Hatlen created issue -
        Mamta A. Satoor made changes -
        Field Original Value New Value
        Labels derby_triage10_9
        Urgency Low [ 10053 ]
        Hide
        Dag H. Wanvik added a comment -

        Uploading a patch which replaces '/' or '\' with underscore when mapping SQL identifiers (schema, jarname) to filename. The patch introduces a new method in FileUtil: sanitizeSqlIdAsFilename

        From its Javadoc: "Since quoted SQL identifiers may contain any character, we cannot use an SQL identifier as a file name unconditionally. Return a safe (unexploitable) file name by replacing '/' or '\' with underscore, so one can't access a non-intended directory. <em>Note</em>: we need to replace both to make database portable from Windows to *nix or vice versa."

        I don't think we need any upgrade logic here: It seems very unlikely anybody relies on this error. If they do, e.g. a '/' on a Windows system, the workaround is simple, just rename the affected jars.

        No tests yet beyond manual, running regressions.

        Show
        Dag H. Wanvik added a comment - Uploading a patch which replaces '/' or '\' with underscore when mapping SQL identifiers (schema, jarname) to filename. The patch introduces a new method in FileUtil: sanitizeSqlIdAsFilename From its Javadoc: "Since quoted SQL identifiers may contain any character, we cannot use an SQL identifier as a file name unconditionally. Return a safe (unexploitable) file name by replacing '/' or '\' with underscore, so one can't access a non-intended directory. <em>Note</em>: we need to replace both to make database portable from Windows to *nix or vice versa." I don't think we need any upgrade logic here: It seems very unlikely anybody relies on this error. If they do, e.g. a '/' on a Windows system, the workaround is simple, just rename the affected jars. No tests yet beyond manual, running regressions.
        Dag H. Wanvik made changes -
        Attachment derby-5357.diff [ 12516773 ]
        Attachment derby-5357.stat [ 12516774 ]
        Dag H. Wanvik made changes -
        Issue & fix info Patch Available [ 10102 ]
        Hide
        Dag H. Wanvik added a comment -

        Changing urgency to normal and ticking the security flag.

        Show
        Dag H. Wanvik added a comment - Changing urgency to normal and ticking the security flag.
        Dag H. Wanvik made changes -
        Bug behavior facts Security [ 10361 ]
        Urgency Low [ 10053 ] Normal [ 10052 ]
        Dag H. Wanvik made changes -
        Assignee Dag H. Wanvik [ dagw ]
        Dag H. Wanvik made changes -
        Status Open [ 1 ] In Progress [ 3 ]
        Hide
        Rick Hillegas added a comment -

        Hi Dag,

        I agree that this vulnerability should be fixed. I believe that these identifiers are used by

        1) sqlj_install_jar

        2) sqlj.remove_jar

        3) sqlj.replace_jar

        4) and in the database classpaths bound to the derby.database.classpath property

        Unfortunately, I believe that your patch can break (2) and (3) for jar files which were stored prior to release 10.9. I don't think that the patch addresses (4).

        Here are some other approaches to consider:

        A) Raise an error if the identifier passed to sqlj.install_jar contains suspicious characters.

        B) Don't touch the identifier stored in SYSFILES.FILENAME. Instead, your regular transformation could be applied to FILENAME at the last possible moment when the file is actually copied, dropped, replaced, or included on the classpath. This approach would need to be used only for jar files which were installed or replaced by 10.9 or later. That, in turn, would require tagging jar files with version information. It might be possible to encode that information in SYSFILES.GENERATIONID.

        C) This is a variation on (B). Build the copied file name from SYSSCHEMAS.SCHEMAID and SYSFILES.FILEID rather than SYSSCHEMAS.SCHEMANAME and SYSFILES.FILENAME. Again, this could only be done for jar files installed/replaced by 10.9 and later releases and would also require some way of tagging the file with a Derby version number.

        Thanks,
        -Rick

        Show
        Rick Hillegas added a comment - Hi Dag, I agree that this vulnerability should be fixed. I believe that these identifiers are used by 1) sqlj_install_jar 2) sqlj.remove_jar 3) sqlj.replace_jar 4) and in the database classpaths bound to the derby.database.classpath property Unfortunately, I believe that your patch can break (2) and (3) for jar files which were stored prior to release 10.9. I don't think that the patch addresses (4). Here are some other approaches to consider: A) Raise an error if the identifier passed to sqlj.install_jar contains suspicious characters. B) Don't touch the identifier stored in SYSFILES.FILENAME. Instead, your regular transformation could be applied to FILENAME at the last possible moment when the file is actually copied, dropped, replaced, or included on the classpath. This approach would need to be used only for jar files which were installed or replaced by 10.9 or later. That, in turn, would require tagging jar files with version information. It might be possible to encode that information in SYSFILES.GENERATIONID. C) This is a variation on (B). Build the copied file name from SYSSCHEMAS.SCHEMAID and SYSFILES.FILEID rather than SYSSCHEMAS.SCHEMANAME and SYSFILES.FILENAME. Again, this could only be done for jar files installed/replaced by 10.9 and later releases and would also require some way of tagging the file with a Derby version number. Thanks, -Rick
        Hide
        Dag H. Wanvik added a comment -

        Thanks, Rick. "Instead, your regular transformation could be applied to FILENAME at the last possible moment when the file is actually copied, dropped, replaced, or included on the classpathInstead, your regular transformation could be applied to FILENAME at the last possible moment when the file is actually copied, dropped, replaced, or included on the classpath" That's what I had in mind I'll research some more.

        Show
        Dag H. Wanvik added a comment - Thanks, Rick. "Instead, your regular transformation could be applied to FILENAME at the last possible moment when the file is actually copied, dropped, replaced, or included on the classpathInstead, your regular transformation could be applied to FILENAME at the last possible moment when the file is actually copied, dropped, replaced, or included on the classpath" That's what I had in mind I'll research some more.
        Hide
        Dag H. Wanvik added a comment -

        ij> select * from sys.sysfiles;
        FILEID |SCHEMAID |FILENAME |GENERATIONID
        -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
        08264012-0135-d0d5-9c5b-0000070b2ff0|80000000-00d2-b38f-4cda-000a0a412c00|../../../x/jar

        I think my code doesn't modify the FILENAME in SYSFILE, cf the select above. Here is the contents of the jar dir in wombat:

        /export/home/dag/wombat/jar/APP:
        total used in directory 390 available 10570418
        :
        rw-rr- 1 dag staff 273331 2012-03-02 01:40 ......_x_jar.jar.G1330648558257

        This mapping would would for all the three sqlj.* stored procs I believe. I'll look some more at your case 4).

        A problem with my approach is, as you say, that if (and only if) '/' or '\' were used in in a SQL id for a jar an old release, it would fail on upgrade: the jar would not be found.

        Another problem is that the mapping in the patch is effectively a hash: there maybe be a chance of collision, e.g. identifiers "a_b" and "a/b" would both map to filename "a_b".

        Show
        Dag H. Wanvik added a comment - ij> select * from sys.sysfiles; FILEID |SCHEMAID |FILENAME |GENERATIONID ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 08264012-0135-d0d5-9c5b-0000070b2ff0|80000000-00d2-b38f-4cda-000a0a412c00|../../../x/jar I think my code doesn't modify the FILENAME in SYSFILE, cf the select above. Here is the contents of the jar dir in wombat: /export/home/dag/wombat/jar/APP: total used in directory 390 available 10570418 : rw-r r - 1 dag staff 273331 2012-03-02 01:40 .. .. .._x_jar.jar.G1330648558257 This mapping would would for all the three sqlj.* stored procs I believe. I'll look some more at your case 4). A problem with my approach is, as you say, that if (and only if) '/' or '\' were used in in a SQL id for a jar an old release, it would fail on upgrade: the jar would not be found. Another problem is that the mapping in the patch is effectively a hash: there maybe be a chance of collision, e.g. identifiers "a_b" and "a/b" would both map to filename "a_b".
        Hide
        Dag H. Wanvik added a comment -

        Btw, Rick's case 4) would be covered by the patch with the change to JarUtil.mkExternalName, too. UpdateLoader#initializeFromClasspath -> JarLoader#initialize- > BasicDatabase#getJarFile -> mkExternalName. But the patch is obviously lacking: we need to tackle the unique mapping problem as well as the upgrade problem, since existing usages of file separators in SQL ids could have been bona fide, e.g. : "a/b", although unlikely as an SQL name, would have worked just fine earlier and should not break on upgrade.

        Show
        Dag H. Wanvik added a comment - Btw, Rick's case 4) would be covered by the patch with the change to JarUtil.mkExternalName, too. UpdateLoader#initializeFromClasspath -> JarLoader#initialize- > BasicDatabase#getJarFile -> mkExternalName . But the patch is obviously lacking: we need to tackle the unique mapping problem as well as the upgrade problem, since existing usages of file separators in SQL ids could have been bona fide, e.g. : "a/b", although unlikely as an SQL name, would have worked just fine earlier and should not break on upgrade.
        Hide
        Mike Matrigali added a comment -

        I think we should consider upgrade problems, even if the previous usage was unlikely. I suggest that any new naming/finding
        scheme be limited to hard upgraded databases, otherwise a user going back to a previous version may "lose" their jars.

        It would be cleaner if somehow a version could be associated with the naming, but I think you still need to maintain the old naming search for old versions. And unless you do some sort of renaming for all existing dbs on hard upgrade, you probably
        need to run both naming schemes in hard upgraded dbs.

        Could somehow the new naming map a new jar to a different "old naming" file? This could lead to removing the wrong jar. This
        is sort of the hashing problem but from new to old rather than new to new.

        It seems like hashing where 2 filenames can collide to a single is a problem, as rick points out. You could eliminate part of the backward compatibility problems by making new databases use a naming scheme that could never collide with with old databases.
        Maybe a new directory (jar2 ?) for new jars in new databases, or maybe there is somethink that could not have been a valid
        SQL identifier and thus could not have been an old filename (would adding a number at the beginning of the filename work?)

        Is basing the new name on the old name a good idea still, maybe
        there should just be a mapping in a catalog to a guaranteed new name, like a uuid or a sequence number.

        Show
        Mike Matrigali added a comment - I think we should consider upgrade problems, even if the previous usage was unlikely. I suggest that any new naming/finding scheme be limited to hard upgraded databases, otherwise a user going back to a previous version may "lose" their jars. It would be cleaner if somehow a version could be associated with the naming, but I think you still need to maintain the old naming search for old versions. And unless you do some sort of renaming for all existing dbs on hard upgrade, you probably need to run both naming schemes in hard upgraded dbs. Could somehow the new naming map a new jar to a different "old naming" file? This could lead to removing the wrong jar. This is sort of the hashing problem but from new to old rather than new to new. It seems like hashing where 2 filenames can collide to a single is a problem, as rick points out. You could eliminate part of the backward compatibility problems by making new databases use a naming scheme that could never collide with with old databases. Maybe a new directory (jar2 ?) for new jars in new databases, or maybe there is somethink that could not have been a valid SQL identifier and thus could not have been an old filename (would adding a number at the beginning of the filename work?) Is basing the new name on the old name a good idea still, maybe there should just be a mapping in a catalog to a guaranteed new name, like a uuid or a sequence number.
        Hide
        Dag H. Wanvik added a comment -

        Thanks, Mike and Rick!
        I bit the bullet. Uploading a new proof-of-concept patch which introduces a new naming scheme, with full upgrade of old jar file storage at hard upgrade time. At soft upgrade time, the old code is still used. Still haven't written tests. I have done away with the schema subdirectories altogether and base the new scheme on uuids:

        Old style file name: jar/<schema>/<name>.jar.<version>
        New style file name: jar/<uuid><safe schema><safe name>.jar.<version>

        I included schema and name (sanitized) in the name for ease of debugging. Perhaps we might remove that if we fear users manually tampering with the stored jars..

        Running regressions.

        Show
        Dag H. Wanvik added a comment - Thanks, Mike and Rick! I bit the bullet. Uploading a new proof-of-concept patch which introduces a new naming scheme, with full upgrade of old jar file storage at hard upgrade time. At soft upgrade time, the old code is still used. Still haven't written tests. I have done away with the schema subdirectories altogether and base the new scheme on uuids: Old style file name: jar/<schema>/<name>.jar.<version> New style file name: jar/<uuid> <safe schema> <safe name>.jar.<version> I included schema and name (sanitized) in the name for ease of debugging. Perhaps we might remove that if we fear users manually tampering with the stored jars.. Running regressions.
        Dag H. Wanvik made changes -
        Attachment derby-5357-2.diff [ 12517094 ]
        Attachment derby-5357-2.stat [ 12517095 ]
        Hide
        Knut Anders Hatlen added a comment -

        I see that the patch uses String.replaceAll(). That method isn't available on CDC/FP. Will that be a problem?

        I'd prefer that we did not include <safe schema> and <safe name> in the file name, as they're not needed and I'm not sure we can guarantee that they are really safe. We remove / and \ from the identifiers, but do we know that there are no other characters that have a special meaning in some file system?

        Show
        Knut Anders Hatlen added a comment - I see that the patch uses String.replaceAll(). That method isn't available on CDC/FP. Will that be a problem? I'd prefer that we did not include <safe schema> and <safe name> in the file name, as they're not needed and I'm not sure we can guarantee that they are really safe. We remove / and \ from the identifiers, but do we know that there are no other characters that have a special meaning in some file system?
        Hide
        Dag H. Wanvik added a comment -

        diffstat and detailed patch comments (derby-5357-2):

        impl/sql/execute/JarUtil.java | 153 +++++++++++++++++++++--
        impl/sql/catalog/DataDictionaryImpl.java | 85 ++++++++++++
        impl/store/raw/RawStore.java | 57 ++++++--
        iapi/sql/dictionary/DataDescriptorGenerator.java | 38 +++--
        iapi/services/io/FileUtil.java | 16 ++
        iapi/store/access/FileResource.java | 5
        impl/db/BasicDatabase.java | 3
        impl/sql/catalog/DD_Version.java | 7 +
        impl/store/raw/data/RFResource.java | 8 +

        M java/engine/org/apache/derby/impl/sql/execute/JarUtil.java

        mkExternalName now always takes uuid as argument.

        New methods:

        • mkExternalNameInternal: used during upgrade to be able to construct both old an new style names
        • upgradeJar: upgrade one jar file to new style (>= 10.9)
        • removeOldDirs: Upgrade code: Remove the old directories names with contents (pre-10.9 style).

        M java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java

        New methods:

        • getAllSysfileDescriptors: used by upgrade code: Return a list of all {@FileInfoDescriptor}

          s in SYSFILES scan.

        • upgradeJarStorage: used by upgrade code. Called by the upgrade code to upgrade the way we store jar files in the database. We now use UUID as part of the file name and sanitize the SQL (schema, schema object) parts of the file name to avoid problems with path delimiters. Also, we henceforth use no schema subdirectories since there is no chance of name collision with the UUID.

        M java/engine/org/apache/derby/impl/store/raw/RawStore.java

        Modified backup code to handle new as well as old scheme (in soft
        upgrade mode).

        M java/engine/org/apache/derby/iapi/services/io/FileUtil.java

        New method:

        • sanitizeSqlIdAsFilename: Since quoted SQL identifiers may contain any character, we cannot use an SQL identifier as a file name unconditionally. Return a safe (unexploitable) file name by replacing '/' or '\' with underscore, so one can't access a non-intended directory. <em>Note</em>: we need to replace both to make database portable from Windows to *nix or vice versa.

        M java/engine/org/apache/derby/iapi/store/access/FileResource.java
        M java/engine/org/apache/derby/impl/store/raw/data/RFResource.java

        New method:

        • getAsFile(String name) to be able to delete old directories during upgrade.

        M java/engine/org/apache/derby/impl/sql/catalog/DD_Version.java

        Add a line to call upgradeJarStorage when hard upgrading

        M java/engine/org/apache/derby/impl/db/BasicDatabase.java

        Interface adjustment.

        M java/engine/org/apache/derby/iapi/sql/dictionary/DataDescriptorGenerator.java

        interface adjustment

        Show
        Dag H. Wanvik added a comment - diffstat and detailed patch comments (derby-5357-2): impl/sql/execute/JarUtil.java | 153 +++++++++++++++++++++-- impl/sql/catalog/DataDictionaryImpl.java | 85 ++++++++++++ impl/store/raw/RawStore.java | 57 ++++++-- iapi/sql/dictionary/DataDescriptorGenerator.java | 38 +++-- iapi/services/io/FileUtil.java | 16 ++ iapi/store/access/FileResource.java | 5 impl/db/BasicDatabase.java | 3 impl/sql/catalog/DD_Version.java | 7 + impl/store/raw/data/RFResource.java | 8 + M java/engine/org/apache/derby/impl/sql/execute/JarUtil.java mkExternalName now always takes uuid as argument. New methods: mkExternalNameInternal: used during upgrade to be able to construct both old an new style names upgradeJar: upgrade one jar file to new style (>= 10.9) removeOldDirs: Upgrade code: Remove the old directories names with contents (pre-10.9 style). M java/engine/org/apache/derby/impl/sql/catalog/DataDictionaryImpl.java New methods: getAllSysfileDescriptors: used by upgrade code: Return a list of all {@FileInfoDescriptor} s in SYSFILES scan. upgradeJarStorage: used by upgrade code. Called by the upgrade code to upgrade the way we store jar files in the database. We now use UUID as part of the file name and sanitize the SQL (schema, schema object) parts of the file name to avoid problems with path delimiters. Also, we henceforth use no schema subdirectories since there is no chance of name collision with the UUID. M java/engine/org/apache/derby/impl/store/raw/RawStore.java Modified backup code to handle new as well as old scheme (in soft upgrade mode). M java/engine/org/apache/derby/iapi/services/io/FileUtil.java New method: sanitizeSqlIdAsFilename: Since quoted SQL identifiers may contain any character, we cannot use an SQL identifier as a file name unconditionally. Return a safe (unexploitable) file name by replacing '/' or '\' with underscore, so one can't access a non-intended directory. <em>Note</em>: we need to replace both to make database portable from Windows to *nix or vice versa. M java/engine/org/apache/derby/iapi/store/access/FileResource.java M java/engine/org/apache/derby/impl/store/raw/data/RFResource.java New method: getAsFile(String name) to be able to delete old directories during upgrade. M java/engine/org/apache/derby/impl/sql/catalog/DD_Version.java Add a line to call upgradeJarStorage when hard upgrading M java/engine/org/apache/derby/impl/db/BasicDatabase.java Interface adjustment. M java/engine/org/apache/derby/iapi/sql/dictionary/DataDescriptorGenerator.java interface adjustment
        Hide
        Dag H. Wanvik added a comment -

        Thanks Knut, You are probably right about replaceAll. As far as other characters, I believe there are at least illegal characters in some file systems that could be seen to give issues, but so could too possible too large an identifier, although probably Derby's maximum identifier length makes our names small enough (16 + 1 + 128 + 1 + 128 + 4 (.jar) + 2 (.G) + maxDigits(long) is still quite a bit....). We could map only alphanumeric perhaps. I tend to agree with you, better remove that part of the name altogether. That removes the replaceAll issue, too..

        Show
        Dag H. Wanvik added a comment - Thanks Knut, You are probably right about replaceAll. As far as other characters, I believe there are at least illegal characters in some file systems that could be seen to give issues, but so could too possible too large an identifier, although probably Derby's maximum identifier length makes our names small enough (16 + 1 + 128 + 1 + 128 + 4 (.jar) + 2 (.G) + maxDigits(long) is still quite a bit....). We could map only alphanumeric perhaps. I tend to agree with you, better remove that part of the name altogether. That removes the replaceAll issue, too..
        Hide
        Dag H. Wanvik added a comment - - edited

        Uploading an enlarged version of this patch with upgrade logic and upgrade tests, and updates to dblook (dblook tests failed before with the new scheme).

        Note: the new code in RawStore had to accommodate the weird test in BackupPathTests.jar, which somewhat worryingly backs up <db> to <db>/jar (gave a recursion problem in the new code).

        I still have not removed the sanitized schema and SQL object names from the file name of the jar, nor the regexp code unwanted for CDC, but will do so in a final increment.

        Regressions passed.

        Show
        Dag H. Wanvik added a comment - - edited Uploading an enlarged version of this patch with upgrade logic and upgrade tests, and updates to dblook (dblook tests failed before with the new scheme). Note: the new code in RawStore had to accommodate the weird test in BackupPathTests.jar, which somewhat worryingly backs up <db> to <db>/jar (gave a recursion problem in the new code). I still have not removed the sanitized schema and SQL object names from the file name of the jar, nor the regexp code unwanted for CDC, but will do so in a final increment. Regressions passed.
        Dag H. Wanvik made changes -
        Attachment derby-5357-with-tests.diff [ 12517513 ]
        Attachment derby-5357-with-tests.stat [ 12517514 ]
        Dag H. Wanvik made changes -
        Attachment derby-5357-with-tests.diff [ 12517513 ]
        Dag H. Wanvik made changes -
        Attachment derby-5357-with-tests.diff [ 12517516 ]
        Dag H. Wanvik made changes -
        Attachment derby-5357-with-tests.diff [ 12517516 ]
        Dag H. Wanvik made changes -
        Attachment derby-5357-with-tests.diff [ 12517518 ]
        Hide
        Dag H. Wanvik added a comment -

        Uploading a hopefully complete version of the patch. This now has dropped the schema/SQL object name in the file name of the stored jar file, and also gets rid of the non-CDC compliant regexp code, Changes10_9 swings its own mini regexp matcher instead.. Regressions passed. Please review.

        Show
        Dag H. Wanvik added a comment - Uploading a hopefully complete version of the patch. This now has dropped the schema/SQL object name in the file name of the stored jar file, and also gets rid of the non-CDC compliant regexp code, Changes10_9 swings its own mini regexp matcher instead.. Regressions passed. Please review.
        Dag H. Wanvik made changes -
        Attachment derby-5357-with-tests-2.diff [ 12518001 ]
        Attachment derby-5357-with-tests-2.stat [ 12518002 ]
        Hide
        Knut Anders Hatlen added a comment -

        During full upgrade we remove the old jar directory before the new dictionary information is committed. Could this cause problems if the database crashes during upgrade (after removal of old jar dir and before committing the upgrade transaction)?

        DataDictionaryImpl.getAllSysfileDescriptors() builds a list of all the rows in SYSFILES. Could this cause memory problems during upgrade if SYSFILES is huge? Would it be safer to combine getAllSysfileDescriptors() and upgradeJarStorage() into a single method so that we don't need the intermediate list? (What you really want is lambda expressions, right?

        Show
        Knut Anders Hatlen added a comment - During full upgrade we remove the old jar directory before the new dictionary information is committed. Could this cause problems if the database crashes during upgrade (after removal of old jar dir and before committing the upgrade transaction)? DataDictionaryImpl.getAllSysfileDescriptors() builds a list of all the rows in SYSFILES. Could this cause memory problems during upgrade if SYSFILES is huge? Would it be safer to combine getAllSysfileDescriptors() and upgradeJarStorage() into a single method so that we don't need the intermediate list? (What you really want is lambda expressions, right?
        Hide
        Dag H. Wanvik added a comment - - edited

        Thanks, Knut!

        > Could this cause problems if the database crashes during upgrade (after removal of old jar dir and before committing the upgrade transaction)

        Yes, it could. I thought of leaving the old jar dirs in place, but that may be no desirable either. I see SQLJ.REMOVE_JAR holds off the physical removal till transaction commit time, maybe I can do something similar during upgrade boot.

        > Could this cause memory problems during upgrade if SYSFILES is huge

        In theory, sure. There is precedent code using this idiom in DataDictionaryImpl, but you're right, I could easily merge the two methods to avoid the issue altogether.

        I also found that the upgrade test code which tests the upgraded jars with "CALL EMC.ADDCONTACT" won't run under CDC/JSR169 (nested connection requires DriverManager), so I'll just skip that part of the test on that platform.

        I'll spin a new rev.

        Show
        Dag H. Wanvik added a comment - - edited Thanks, Knut! > Could this cause problems if the database crashes during upgrade (after removal of old jar dir and before committing the upgrade transaction) Yes, it could. I thought of leaving the old jar dirs in place, but that may be no desirable either. I see SQLJ.REMOVE_JAR holds off the physical removal till transaction commit time, maybe I can do something similar during upgrade boot. > Could this cause memory problems during upgrade if SYSFILES is huge In theory, sure. There is precedent code using this idiom in DataDictionaryImpl, but you're right, I could easily merge the two methods to avoid the issue altogether. I also found that the upgrade test code which tests the upgraded jars with "CALL EMC.ADDCONTACT" won't run under CDC/JSR169 (nested connection requires DriverManager), so I'll just skip that part of the test on that platform. I'll spin a new rev.
        Hide
        Dag H. Wanvik added a comment -

        Uploading derby-5357-with-tests-3 to handle the three issues mention above. Rerunning regressions.

        Show
        Dag H. Wanvik added a comment - Uploading derby-5357-with-tests-3 to handle the three issues mention above. Rerunning regressions.
        Dag H. Wanvik made changes -
        Attachment derby-5357-with-tests-3.diff [ 12518225 ]
        Attachment derby-5357-with-tests-3.stat [ 12518226 ]
        Hide
        Dag H. Wanvik added a comment -

        A comment on the transaction safety of the removal of the old style jar files (and directories): We now schedule the removal as post-commit work, cf. the new method

        {FileREsource/RFResource}

        #removeJarDir.

        Show
        Dag H. Wanvik added a comment - A comment on the transaction safety of the removal of the old style jar files (and directories): We now schedule the removal as post-commit work, cf. the new method {FileREsource/RFResource} #removeJarDir.
        Hide
        Dag H. Wanvik added a comment -

        Regressions passed. I'll look to commit the patch by end of Friday CET unless there are further comments. Does this issue require a release note? We do change the internal storage format in a way that is visible to the user looking inside the "jar" directory, but then again, I don't believe that is/should be documented?

        Show
        Dag H. Wanvik added a comment - Regressions passed. I'll look to commit the patch by end of Friday CET unless there are further comments. Does this issue require a release note? We do change the internal storage format in a way that is visible to the user looking inside the "jar" directory, but then again, I don't believe that is/should be documented?
        Hide
        Knut Anders Hatlen added a comment -

        With the patch, dblook doesn't work with databases that have been soft-upgraded to 10.9 if they contain jar files. dblook.log says:

        – **--> DEBUG: Failed to load jar file /tmp/jartest/DBJARS/23ce809c-0136-10ee-5e54-0000037b37f8.jar.G1331723919074
        java.io.FileNotFoundException: db/jar/23ce809c-0136-10ee-5e54-0000037b37f8.jar.G1331723919074 (No such file or directory)
        at java.io.FileInputStream.open(Native Method)
        at java.io.FileInputStream.<init>(FileInputStream.java:120)
        at java.io.FileInputStream.<init>(FileInputStream.java:79)
        at org.apache.derby.impl.tools.dblook.DB_Jar.doJars(DB_Jar.java:98)
        at org.apache.derby.tools.dblook.go(dblook.java:533)
        at org.apache.derby.tools.dblook.<init>(dblook.java:142)
        at org.apache.derby.tools.dblook.main(dblook.java:97)
        at org.apache.derby.iapi.tools.run.main(run.java:57)

        Show
        Knut Anders Hatlen added a comment - With the patch, dblook doesn't work with databases that have been soft-upgraded to 10.9 if they contain jar files. dblook.log says: – **--> DEBUG: Failed to load jar file /tmp/jartest/DBJARS/23ce809c-0136-10ee-5e54-0000037b37f8.jar.G1331723919074 java.io.FileNotFoundException: db/jar/23ce809c-0136-10ee-5e54-0000037b37f8.jar.G1331723919074 (No such file or directory) at java.io.FileInputStream.open(Native Method) at java.io.FileInputStream.<init>(FileInputStream.java:120) at java.io.FileInputStream.<init>(FileInputStream.java:79) at org.apache.derby.impl.tools.dblook.DB_Jar.doJars(DB_Jar.java:98) at org.apache.derby.tools.dblook.go(dblook.java:533) at org.apache.derby.tools.dblook.<init>(dblook.java:142) at org.apache.derby.tools.dblook.main(dblook.java:97) at org.apache.derby.iapi.tools.run.main(run.java:57)
        Hide
        Dag H. Wanvik added a comment - - edited

        Thanks for spotting that, Knut. Uploading rev #4 which addresses this problem. I believe we don't have tests that stress dblook in a soft upgrade mode, so this is a hole we may want to address. I tested it manually Rerunning regressions.

        Show
        Dag H. Wanvik added a comment - - edited Thanks for spotting that, Knut. Uploading rev #4 which addresses this problem. I believe we don't have tests that stress dblook in a soft upgrade mode, so this is a hole we may want to address. I tested it manually Rerunning regressions.
        Dag H. Wanvik made changes -
        Attachment derby-5357-with-tests-4.diff [ 12518487 ]
        Attachment derby-5357-with-tests-4.stat [ 12518488 ]
        Hide
        Dag H. Wanvik added a comment -

        Committed patch derby-5357-with-tests-4 as svn 1302836, resolving. Not planning to backport this since it changes the database format.

        Show
        Dag H. Wanvik added a comment - Committed patch derby-5357-with-tests-4 as svn 1302836, resolving. Not planning to backport this since it changes the database format.
        Dag H. Wanvik made changes -
        Status In Progress [ 3 ] Resolved [ 5 ]
        Issue & fix info Patch Available [ 10102 ]
        Fix Version/s 10.9.0.0 [ 12316344 ]
        Resolution Fixed [ 1 ]
        Knut Anders Hatlen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Kathey Marsden added a comment -

        Reopening to label as not appropriate for backport to 10.8

        Show
        Kathey Marsden added a comment - Reopening to label as not appropriate for backport to 10.8
        Kathey Marsden made changes -
        Resolution Fixed [ 1 ]
        Status Closed [ 6 ] Reopened [ 4 ]
        Kathey Marsden made changes -
        Labels derby_triage10_9 derby_backport_reject_10_8 derby_triage10_9
        Hide
        Kathey Marsden added a comment -

        Reresolving. Issue not backportable to 10.8 because of database format change

        Show
        Kathey Marsden added a comment - Reresolving. Issue not backportable to 10.8 because of database format change
        Kathey Marsden made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Knut Anders Hatlen made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Gavin made changes -
        Workflow jira [ 12623840 ] Default workflow, editable Closed status [ 12802900 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open In Progress In Progress
        217d 13h 51m 1 Dag H. Wanvik 02/Mar/12 01:33
        In Progress In Progress Resolved Resolved
        18d 9h 33m 1 Dag H. Wanvik 20/Mar/12 11:06
        Closed Closed Reopened Reopened
        49d 50m 1 Kathey Marsden 11/Sep/12 19:18
        Reopened Reopened Resolved Resolved
        1m 1s 1 Kathey Marsden 11/Sep/12 19:19
        Resolved Resolved Closed Closed
        404d 21h 28m 2 Knut Anders Hatlen 17/Jun/13 10:27

          People

          • Assignee:
            Dag H. Wanvik
            Reporter:
            Knut Anders Hatlen
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development