r89252 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r89251‎ | r89252 | r89253 >
Date:08:46, 1 June 2011
Author:freakolowsky
Status:resolved (Comments)
Tags:
Comment:
* MFT r89250. only the tableExists function ad 1.17 already supports user-dbname difference
Modified paths:
  • /branches/REL1_17/phase3/includes/db/DatabaseOracle.php (modified) (history)

Diff [purge]

Index: branches/REL1_17/phase3/includes/db/DatabaseOracle.php
——@@ -936,8 +936,9 @@
937937 * Query whether a given table exists (in the given schema, or the default mw one if not given)
938938 */
939939 function tableExists( $table ) {
940 - $table = trim($this->tableName($table), '"');
941 - $SQL = "SELECT 1 FROM user_tables WHERE table_name='$table'";
 940+ $table = trim( $this->tableName($table), '"' );
 941+ $owner = strtoupper( $this->mDBname );
 942+ $SQL = "SELECT 1 FROM all_tables WHERE owner='$owner' AND table_name='$table'";
942943 $res = $this->doQuery( $SQL );
943944 if ( $res ) {
944945 $count = $res->numRows();
——@@ -945,7 +946,7 @@
946947 } else {
947948 $count = 0;
948949 }
949 - return $count;
 950+ return $count!=0;
950951 }
951952
952953 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r89759* wrapped table and dbname with addQuotesfreakolowsky07:15, 9 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r89250* upgrade patches for oracle 1.17->1.19...freakolowsky08:27, 1 June 2011

Comments

#Comment by Tim Starling (talk | contribs)   05:28, 8 June 2011

Lacks proper string escaping in SQL construction.

I'm not sure what to write for this in the release notes, is it fixing a bug?

#Comment by Freakolowsky (talk | contribs)   07:12, 8 June 2011

The function is not exposed to direct input so i don't see any reason why to even bother with it.

This change was made because we introduced selectDB functionality into this backend type. What we do there is change the default schema pointer so DB has me logged in as one user but uses objects in another schema as my defaults. However all the user_* catalogs still show the objects from the actual user so we have to use main catalogs.

#Comment by Tim Starling (talk | contribs)   23:28, 8 June 2011

It's easier to add a $this->addQuotes() than to confirm that it is secure by following the data flow in every place where it is used and confirming that there's no way for user input to find its way into this function. That's why our security policy is to always escape, regardless of the data source.

As for the release notes, I'm asking if there is some user-visible consequence of fixing tableExists(). For instance, does it avoid an error message on install or upgrade?

#Comment by Freakolowsky (talk | contribs)   07:17, 9 June 2011

r89759.

Without this fix updating on installs where username != dbname will not work as it looks for tables in the wrong place and always tries to install instead of upgrade.

Status & tagging log