org.apache.derby.impl.load.Import needs to escape single quotes

• Key: DERBY-4042
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Knut Anders Hatlen
• Reporter: Knut Anders Hatlen
• Component/s: Tools
• Affects Version/s: 10.4.2.0
• Fix Version/s: 10.5.1.1

Description

The code that builds the SQL statement that invokes the Import VTI doesn't properly escape single quotes. This causes problems for users, see: http://mail-archives.apache.org/mod_mbox/db-derby-user/200901.mbox/[e-mail]%3e

Import.performImport() is the method that needs to be fixed.

Comments

[Knut Anders Hatlen]

There are users who have come across this issue and have found out how to trick Derby into accepting strings with quotes in them, so I'm flagging the issue with "Existing Application Impact" and "Release Note Needed", since those users will have to remove the extra escape characters from their applications after the issue has been fixed.

[Knut Anders Hatlen]

Attached is a patch (d4042-1.diff) that makes the export and import procedures quote arguments correctly by using helper methods in IdUtil and StringUtil instead of just putting quotes around them. The patch also adds a test case to ImportExportTest (the test case is small, but the test diff is somewhat big because many helper methods had to be changed to allow another schema than APP to be used).

I have only run the tools test so far, but I will start the full regression suite soon.

I also believe there are some problems in ColumnInfo if the column names in the table to export/import contain double quotes, but I'll defer that to a followup patch.

[Bryan Pendleton]

The patch looks great, Knut. Thank goodness for IdUtil and StringUtil, they
are very useful. I had a little bit of struggle with the test, because I could see
where you added a test for quotes in schema name / table name (the IdUtil case),
but I couldn't see where you added a test for quotes in the file name (the StringUtil case).

Did I simply overlook that test?

[Knut Anders Hatlen]

Thanks for looking at the patch, Bryan!

There is only one added test case (testQuotesInArguments) and it actually does test quotes in the file name, although somewhat indirectly. The helper method doExport() exports a table to a file whose name is tableName + ".dat", and doImportAndVerify() imports a file with the same name. So since the table name contains both a single quote and a double quote, the name of the exported file should contain those characters too. It's probably worth a comment in the test case to make it clearer.

[Bryan Pendleton]

Thanks for the clarification. That seems fine to me. Yes, a comment in the test case would be worthwhile.

[Knut Anders Hatlen]

Added the following comment and committed revision 739830:

        // Create schema names and table names containing both single quotes
        // and double quotes to expose bugs both for internally generated
        // string literals (enclosed in single quotes) and SQL identifiers
        // (enclosed in double quotes). This will also indirectly test that
        // the export/import system procedures handle those characters in
        // file names (which they didn't do very well before the fix for
        // DERBY-4042), as doExport() and doImportAndVerify() use a file name
        // derived from the table name.

[Myrna van Lunteren]

It appears the commit caused failures on windows platforms, see for instance: http://dbtg.thresher.com/derby/test/Daily/jvm1.5/testing/Limited/testSummary-740039.html

Can you investigate?

[Knut Anders Hatlen]

I don't have a Windows box to test it on, but it looks like Windows isn't too happy with double quotes in file names. The bug only involved file names with single quotes, so I guess it would be fine to rewrite the test to test file names with single quotes only.

[Knut Anders Hatlen]

The attached patch changes the test so that it doesn't use a double quote in the file name. I haven't tested it on Windows, but I hope that this will make the test pass on that platform too.

Committed revision 740698.

[Myrna van Lunteren]

It passes for me on XP.
I was thinking maybe we could add some os-switching logic, but this is ok.

[Chip Hartney]

Windows does not all any of the following characters in file names: \ / : * ? " | < >

Addressing the apost is sufficient for me, at least.

[Knut Anders Hatlen]

Here's an attempt on a release note for this issue.