Bug Hunting Session
Bug 67302 - TablesSupplier name clash when dots in schema/table name
Summary: TablesSupplier name clash when dots in schema/table name
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyInteresting, easyHack, skillCpp, skillSql
Depends on:
Blocks:
 
Reported: 2013-07-25 13:27 UTC by Lionel Elie Mamane
Modified: 2017-02-14 08:57 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Attaching changes made in some file for escaped schema name and table name. Tell me I am going in right direction. (7.44 KB, patch)
2016-05-05 07:20 UTC, Prashant
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Elie Mamane 2013-07-25 13:27:25 UTC
The implementations of interface XTablesSupplier.getTables() in LibreOffice currently return a XNameAccess that uses:
  schemaName + "." + tableName
as "name" to access the tables.

However, this scheme can make collisions between
 schemaName == "foo"
 tableName == "bar.qux"
and
 schemaName == "foo.bar"
 tableName == "qux"

because in both cases, the name resolves to "foo.bar.qux".

The XNameAccess should rather use properly quoted and escaped names, that is:
 '"' + escapeDoubleQuotes(schemaName) + '"."' + escapeDoubleQuotes(tableName) + '"'

double quotes should be escaped by repeating them.

At the same time, all callers to this interface have to be changed, too. The whole name construction business should be done by a utility function, most probably ::dbtools::composeTableNameForSelect (which needs to be changed to handle double quotes in names...).


Implementations of XTablesSupplier:

 1) MySQL driver: I haven't found where the name is constructed; possibly
    in db-agnostic dbaccess/ or connectivity/source/sdbcx or connectivity/source/commontools
    code.

 2) PostgreSQL driver: pq_xtables.cxx function Tables::refresh code snippet:

                buf.append( schema ).appendAscii( "." ).append( name );
                map[ buf.makeStringAndClear() ] = currentTableIndex;


    callers that need to be also modified:
    pq_resultsetmetadata.cxx function ResultSetMetaData::checkTable
Comment 1 Björn Michaelsen 2013-10-04 18:47:14 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 2 Alex Thurgood 2015-01-03 17:40:20 UTC Comment hidden (no-value)
Comment 3 Robinson Tryon (qubit) 2015-12-13 13:24:13 UTC Comment hidden (obsolete)
Comment 4 Robinson Tryon (qubit) 2016-02-18 14:51:40 UTC Comment hidden (obsolete)
Comment 5 Prashant 2016-05-01 07:07:18 UTC
I find this task interesting, I want to solve it as my first easy hack. Can anyone please give me some pointer to file to start working on it so that I can assign this task to myself.
Comment 6 Lionel Elie Mamane 2016-05-01 08:09:12 UTC
In connectivity/source/drivers/*, each directory should contain one implementation of getTables() from the XTablesSupplier interface. This does not concern the getTables from the XDatabaseMetaData interface.

See
  http://api.libreoffice.org/docs/idl/ref/interfacecom_1_1sun_1_1star_1_1sdbc_1_1XDatabaseMetaData.html#aee9c7e3149e149b3694c900bd6a0671f
  http://api.libreoffice.org/docs/idl/ref/interfacecom_1_1sun_1_1star_1_1sdbcx_1_1XTablesSupplier.html#a364c9ed144725a9c2fdb159e2c201e19

The easy way to make the difference between the two is that the one we care about in this bug does not take any arguments, while the other one does. So we need to look for (e.g. with "git grep") getTables definitions and invocations without any argument.

Each needs to be changed. Then everywhere getTables (without arguments) is *called* needs to be changed.

For the first part, the likely matches are:
 connectivity/source/drivers/evoab2/NCatalog.cxx
 connectivity/source/drivers/kab/KCatalog.cxx
 connectivity/source/drivers/macab/MacabCatalog.cxx
 connectivity/source/drivers/mork/MCatalog.cxx
 connectivity/source/drivers/postgresql/pq_connection.cxx
 connectivity/source/sdbcx/VCatalog.cxx
 dbaccess/source/core/api/SingleSelectQueryComposer.cxx
 dbaccess/source/core/api/querycomposer.cxx
 dbaccess/source/core/dataaccess/connection.cxx
 dbaccess/source/core/dataaccess/datasource.cxx

When these return a pre-made object (which I expect to be common), you need to go look where that object is created. For example, according to comment 0, for postgresql/pq_connection.cxx, this is in pq_xtables.cxx function Tables::refresh

I suggest you take them one by one, and we can discuss them one by one.
Comment 7 Prashant 2016-05-01 11:30:39 UTC
Assigning to myself
Comment 8 Prashant 2016-05-05 07:20:00 UTC
Created attachment 124855 [details]
Attaching changes made in some file for escaped schema name and table name. Tell me I am going in right direction.

I have attached patch file for postgre driver. I am not sure if I am going in right direction that's why uploading patch here. Currently not followed all coding conventions. Please tell me changes so that I can make it better.
Comment 9 jani 2016-05-05 07:22:57 UTC
(In reply to Prashant from comment #8)
> Created attachment 124855 [details]
> Attaching changes made in some file for escaped schema name and table name.
> Tell me I am going in right direction.
> 
> I have attached patch file for postgre driver. I am not sure if I am going
> in right direction that's why uploading patch here. Currently not followed
> all coding conventions. Please tell me changes so that I can make it better.

Please submit the patch to gerrit, that is where we do reviews.


have a look at:

https://wiki.documentfoundation.org/Development/GetInvolved

and ping me if you need help.
Comment 10 Lionel Elie Mamane 2016-05-05 08:44:26 UTC
(In reply to jan iversen from comment #9)

> Please submit the patch to gerrit, that is where we do reviews.

When you do that, please:
1) clearly mark it as WiP (Work in Progress),
   as long as you don't think it ready for merging,
   only looking for feedback.
2) add me as reviewer

(In reply to Prashant from comment #8)
> Created attachment 124855 [details]
> Attaching changes made in some file for escaped schema name and table name.
> Tell me I am going in right direction.

From a first glance, yes it looks like going in the right direction.

However, instead of quoting by calling
  .replaceAll("\"","\"\"" )
I'd suggest using ::dbtools::composeTableNameForSelect
(see include/connectivity/dbtools.hxx)
For the case of PostgreSQL, since it does not support catalogs, just pass an empty catalog.

In pq_resultsetmetadata.cxx: good, this is indeed a name access on an object returned by getTables()

The changes in pq_statement.cxx do not look to me like they belong to this bug, nor are they consistent:

1) In function executePostgresCommand, you change the treatment of sourceTable, but sourceTable comes from extractSingleTableFromSelect, and I don't see you adding any escaping there. Actually, reading the code, extractSingleTableFromSelect removes the enclosing '"' without undoubling the inner '"', so it looks like another related bug there. If you want, you can fix that other bug in a separate patch (upload it to gerrit and put me as reviewer).

2) In function getGeneratedValuesFromLastInsert, you change the treatment of the argument lastTableInserted, but I don't see you changing the value passed. Unless I have missed something, this is inconsistent


pq_tools.cxx function splitDoubleQuoteEscapedIdentifiers: why do you go through an OString instead of doing everything on OUStrings directly?
Comment 11 jani 2016-06-06 06:02:15 UTC
A polite ping, still working on this issue ?
Comment 12 Commit Notification 2016-06-23 10:16:13 UTC
Prashant committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=d43f4390e006716ca538ad92d09bd012179efd66

tdf#67302 Resolving tablesSupplier name clash for postgresql

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 13 jani 2016-06-24 06:46:02 UTC
Seems solved
Comment 14 Commit Notification 2016-08-07 16:54:28 UTC
Lionel Elie Mamane committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b6976604ca15259af3a3ee95e10d24937bd63b9a

Revert "tdf#67302 Resolving tablesSupplier name clash for postgresql"

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 15 Julien Nabet 2016-08-07 16:54:50 UTC
Following the revert (see https://cgit.freedesktop.org/libreoffice/core/commit/?id=b6976604ca15259af3a3ee95e10d24937bd63b9a), let's reopen this one.