r77713 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r77712‎ | r77713 | r77714 >
Date:09:27, 4 December 2010
Author:dantman
Status:ok (Comments)
Tags:
Comment:
Fix a bug in the installer caused by r77487 creating installer sql statements like "GRANT ALL PRIVILEGES ON `'dbname'`.* TO ''tablename''@'%" while improving the database independence of replaceVars.

* Drop unused and likely broken /*$var*/` -> `$var syntax
* Replace {$var} with '{$var}' and `{$var}` handling that uses relevant database independent quoting ({$var} without surrouding quotes are never used)
* Give the generic/mysql class a proper quote_ident implementation
* Fix the unused Oracle and Sqlite quote_ident implementations which are potential sql injections if used
* Split common variable replacemnt code off to a replaceGlobalVars and make the generic and oracle code use it instead of duplicating the same code as each other
Modified paths:
  • /trunk/phase3/includes/db/Database.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseOracle.php (modified) (history)
  • /trunk/phase3/includes/db/DatabasePostgres.php (modified) (history)
  • /trunk/phase3/includes/db/DatabaseSqlite.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/db/DatabaseOracle.php
——@@ -1126,7 +1126,7 @@
11271127 }
11281128
11291129 function quote_ident( $s ) {
1130 - return $s;
 1130+ return '"' . str_replace( '"', '""', $s ) . '"';
11311131 }
11321132
11331133 function selectRow( $table, $vars, $conds, $fname = 'DatabaseOracle::selectRow', $options = array(), $join_conds = array() ) {
——@@ -1345,15 +1345,7 @@
13461346 $varnames[] = '_OracleTempTS';
13471347 }
13481348
1349 - // Ordinary variables
1350 - foreach ( $varnames as $var ) {
1351 - if ( isset( $GLOBALS[$var] ) ) {
1352 - $val = $this->addQuotes( $GLOBALS[$var] ); // FIXME: safety check?
1353 - $ins = str_replace( '{$' . $var . '}', $val, $ins );
1354 - $ins = str_replace( '/*$' . $var . '*/`', '`' . $val, $ins );
1355 - $ins = str_replace( '/*$' . $var . '*/', $val, $ins );
1356 - }
1357 - }
 1349+ $ins = $this->replaceGlobalVars( $ins, $varnames );
13581350
13591351 return parent::replaceVars( $ins );
13601352 }
Index: trunk/phase3/includes/db/DatabasePostgres.php
——@@ -973,7 +973,7 @@
974974 * Return the next in a sequence, save the value for retrieval via insertId()
975975 */
976976 function nextSequenceValue( $seqName ) {
977 - $safeseq = preg_replace( "/'/", "''", $seqName );
 977+ $safeseq = str_replace( "'", "''", $seqName );
978978 $res = $this->query( "SELECT nextval('$safeseq')" );
979979 $row = $this->fetchRow( $res );
980980 $this->mInsertId = $row[0];
——@@ -984,7 +984,7 @@
985985 * Return the current value of a sequence. Assumes it has been nextval'ed in this session.
986986 */
987987 function currentSequenceValue( $seqName ) {
988 - $safeseq = preg_replace( "/'/", "''", $seqName );
 988+ $safeseq = str_replace( "'", "''", $seqName );
989989 $res = $this->query( "SELECT currval('$safeseq')" );
990990 $row = $this->fetchRow( $res );
991991 $currval = $row[0];
——@@ -1242,7 +1242,7 @@
12431243 * Query whether a given schema exists. Returns the name of the owner
12441244 */
12451245 function schemaExists( $schema ) {
1246 - $eschema = preg_replace( "/'/", "''", $schema );
 1246+ $eschema = str_replace( "'", "''", $schema );
12471247 $SQL = "SELECT rolname FROM pg_catalog.pg_namespace n, pg_catalog.pg_roles r "
12481248 ."WHERE n.nspowner=r.oid AND n.nspname = '$eschema'";
12491249 $res = $this->query( $SQL );
——@@ -1301,7 +1301,7 @@
13021302 }
13031303
13041304 function quote_ident( $s ) {
1305 - return '"' . preg_replace( '/"/', '""', $s ) . '"';
 1305+ return '"' . str_replace( '"', '""', $s ) . '"';
13061306 }
13071307
13081308 /**
Index: trunk/phase3/includes/db/Database.php
——@@ -1695,6 +1695,15 @@
16961696 }
16971697
16981698 /**
 1699+ * Quotes a string using `backticks` for things like database, table, and field
 1700+ * names, other databases which use something other than backticks can replace
 1701+ * this with something else
 1702+ */
 1703+ function quote_ident( $s ) {
 1704+ return "`" . $this->strencode( $s ) . "`";
 1705+ }
 1706+
 1707+ /**
16991708 * Escape string for safe LIKE usage.
17001709 * WARNING: you should almost never use this function directly,
17011710 * instead use buildLike() that escapes everything automatically
——@@ -2501,6 +2510,32 @@
25022511 }
25032512
25042513 /**
 2514+ * Database independent variable replacement, replaces a set of named variables
 2515+ * in a sql statement with the contents of their global variables.
 2516+ * Supports '{$var}' `{$var}` and / *$var* / (without the spaces) style variables
 2517+ *
 2518+ * '{$var}' should be used for text and is passed through the database's addQuotes method
 2519+ * `{$var}` should be used for identifiers (eg: table and database names), it is passed through
 2520+ * the database's quote_ident method which can be overridden if the database
 2521+ * uses something other than backticks.
 2522+ * / *$var* / is just encoded, besides traditional dbprefix and tableoptions it's use should be avoided
 2523+ *
 2524+ * @param $ins String: SQL statement to replace variables in
 2525+ * @param $varnames Array: Array of global variable names to replace
 2526+ * @return String The new SQL statement with variables replaced
 2527+ */
 2528+ protected function replaceGlobalVars( $ins, $varnames ) {
 2529+ foreach ( $varnames as $var ) {
 2530+ if ( isset( $GLOBALS[$var] ) ) {
 2531+ $ins = str_replace( '\'{$' . $var . '}\'', $this->addQuotes( $GLOBALS[$var] ), $ins ); // replace '{$var}'
 2532+ $ins = str_replace( '`{$' . $var . '}`', $this->quote_ident( $GLOBALS[$var] ), $ins ); // replace `{$var}`
 2533+ $ins = str_replace( '/*$' . $var . '*/', $this->strencode( $GLOBALS[$var] ) , $ins ); // replace /*$var*/
 2534+ }
 2535+ }
 2536+ return $ins;
 2537+ }
 2538+
 2539+ /**
25052540 * Replace variables in sourced SQL
25062541 */
25072542 protected function replaceVars( $ins ) {
——@@ -2510,15 +2545,7 @@
25112546 'wgDBadminuser', 'wgDBadminpassword', 'wgDBTableOptions',
25122547 );
25132548
2514 - // Ordinary variables
2515 - foreach ( $varnames as $var ) {
2516 - if ( isset( $GLOBALS[$var] ) ) {
2517 - $val = $this->addQuotes( $GLOBALS[$var] ); // FIXME: safety check?
2518 - $ins = str_replace( '{$' . $var . '}', $val, $ins );
2519 - $ins = str_replace( '/*$' . $var . '*/`', '`' . $val, $ins );
2520 - $ins = str_replace( '/*$' . $var . '*/', $val, $ins );
2521 - }
2522 - }
 2549+ $ins = $this->replaceGlobalVars( $ins, $varnames );
25232550
25242551 // Table prefixes
25252552 $ins = preg_replace_callback( '!/\*(?:\$wgDBprefix|_)\*/([a-zA-Z_0-9]*)!',
Index: trunk/phase3/includes/db/DatabaseSqlite.php
——@@ -531,7 +531,7 @@
532532 }
533533
534534 function quote_ident( $s ) {
535 - return $s;
 535+ return '"' . str_replace( '"', '""', $s ) . '"';
536536 }
537537
538538 function buildLike() {

Follow-up revisions

RevisionCommit summaryAuthorDate
r77720Followup to r77713, rename quote_ident to addIdentifierQuotes to follow namin...dantman15:14, 4 December 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r77487Replace addslashes with Database::addQuotes()platonides18:21, 30 November 2010

Comments

#Comment by Dantman (talk | contribs)   10:00, 4 December 2010

Oracle and SQLite quote_ident implementation was checked before implementation:

This line was very odd: 

$ins = str_replace( '/*$' . $var . '*/`', '`' . $val, $ins );

Looking at it it appeared to be prepared to replace something like '`/*$wgDBprefix*/`' with '``mw_' or '/*$wgDBprefix*/`' with '`mw_' which looked extremely odd, and not implemented properly. Checking the contents of our .sql files that syntax does not appear to be present in any of them so since it's unused and improperly implemented I dropped it.

Then again perhaps it was intended to replace /*$wgDBprefix*/`page` with `mw_page` but that's not even used anyways. Generic replaceVars does not replace $wgDBprefix, a separate statement replaces /*$wgDBprefix*/page in generic code, and oracle only uses '{$wgDBprefix}' and /*$wgDBprefix*/testitem.

r77487 caused a bug in '{$wgDBuser}' and `{$wgDBname}` inside of users.sql previously {$wg...} would simply be replaced with escaped data. But since addQuotes was being used instead we got things like foo and `'foo'` instead of 'foo' and `foo` as we expected. This broke the installer if you tried to install as a superuser and asked MediaWiki to create a new database user for the wiki because users.sql would output a query like "GRANT ALL PRIVILEGES ON `'dbname'`.* TO tablename@'%'" prompting a syntax error response from the database. That also had the potential to be a installer based sql injection bug. In pursuit of better database independent functionality, and wanting to avoid any sql injections caused by using the wrong kind of quoting instead of switching {$var} back to simply escaping I switched to replacing '{$var}' and `{$var}` with the the variable data passed through addQuotes and quote_ident which now have proper database independent implementations. So in sqlite, etc... where double quotes are used instead of backticks `{$wgDBname}` may become "foo" where it outputs `foo` in MySQL. The only downside here is that 'foo{$var}foo' will not be replaced. And that style of var is not used anywhere in current code. I can't even think of a valid use case for that so far with the variables we are replacing.

DatabasePostgres was also using preg_replace for str_replace jobs replacing ' or " with or "" where every other database class was using str_replace so I also fixed the consistency there.

#Comment by Nikerabbit (talk | contribs)   14:04, 4 December 2010

Would be nice if quote_ident followed our method naming conventions.

PS: The formatter ate your ''s in the above comment.

#Comment by Dantman (talk | contribs)   14:09, 4 December 2010

ya, I know about the formatter at the end... can't edit comments here.

I originally started with addBackticks, then debated switching to something like addIdentityQuotes, but after getting into the postgres, sqlite, oracle, and mssql code I found that quot_ident already existed and was basically the exact same thing I was trying to create so I fixed it up.

I suppose I could deprecate quote_ident and make it call a addIdentityQuotes.

#Comment by Dantman (talk | contribs)   15:14, 4 December 2010

Ok, renamed in r77720

Status & tagging log