From c0bdbd9d81eb22ded95cd7ea9b26a0c36cfa1be0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 10 Jun 2013 12:56:45 +0200 Subject: [PATCH 1/3] introduce and use executeAudited in db.php --- lib/db.php | 150 ++++++++++++++++++++++++++++++++--------------- lib/template.php | 21 +++++++ 2 files changed, 124 insertions(+), 47 deletions(-) diff --git a/lib/db.php b/lib/db.php index 6183655183..ccbfcf3ab5 100644 --- a/lib/db.php +++ b/lib/db.php @@ -23,7 +23,8 @@ class DatabaseException extends Exception{ private $query; - public function __construct($message, $query){ + //FIXME getQuery seems to be unused, maybe use parent constructor with $message, $code and $previous + public function __construct($message, $query = null){ parent::__construct($message); $this->query = $query; } @@ -391,10 +392,63 @@ class OC_DB { return $result; } + /** + * @brief execute a prepared statement, on error write log and throw exception + * @param mixed $stmt PDOStatementWrapper | MDB2_Statement_Common , + * an array with 'sql' and optionally 'limit' and 'offset' keys + * .. or a simple sql query string + * @param array $parameters + * @return result + * @throws DatabaseException + */ + static public function executeAudited( $stmt, array $parameters = null) { + if (is_string($stmt)) { + // convert to an array with 'sql' + if (stripos($stmt,'LIMIT') !== false) { //OFFSET requires LIMIT, se we only neet to check for LIMIT + // TODO try to convert LIMIT OFFSET notation to parameters, see fixLimitClauseForMSSQL + $message = 'LIMIT and OFFSET are forbidden for portability reasons,' + . ' pass an array with \'limit\' and \'offset\' instead'; + \OCP\Util::writeLog('db', $message, \OCP\Util::FATAL); + throw new DatabaseException($message); + } + $stmt = array('sql' => $stmt, 'limit' => null, 'offset' => null); + } + if (is_array($stmt)){ + // convert to prepared statement + if ( ! array_key_exists('sql', $stmt) ) { + $message = 'statement array must at least contain key \'sql\''; + \OCP\Util::writeLog('db', $message, \OCP\Util::FATAL); + throw new DatabaseException($message); + } + if ( ! array_key_exists('limit', $stmt) ) { + $stmt['limit'] = null; + } + if ( ! array_key_exists('limit', $stmt) ) { + $stmt['offset'] = null; + } + $stmt = self::prepare($stmt['sql'], $stmt['limit'], $stmt['offset']); + } + self::raiseExceptionOnError($stmt, 'Could not prepare statement'); + if ($stmt instanceof PDOStatementWrapper || $stmt instanceof MDB2_Statement_Common) { + $result = $stmt->execute($parameters); + self::raiseExceptionOnError($result, 'Could not execute statement'); + } else { + if (is_object($stmt)) { + $message = 'Expected a prepared statement or array got ' . get_class($stmt); + } else { + $message = 'Expected a prepared statement or array got ' . gettype($stmt); + } + \OCP\Util::writeLog('db', $message, \OCP\Util::FATAL); + throw new DatabaseException($message); + } + return $result; + } + /** * @brief gets last value of autoincrement * @param string $table The optional table name (will replace *PREFIX*) and add sequence suffix * @return int id + * @throws DatabaseException * * MDB2 lastInsertID() * @@ -404,25 +458,27 @@ class OC_DB { public static function insertid($table=null) { self::connect(); $type = OC_Config::getValue( "dbtype", "sqlite" ); - if( $type == 'pgsql' ) { - $query = self::prepare('SELECT lastval() AS id'); - $row = $query->execute()->fetchRow(); + if( $type === 'pgsql' ) { + $result = self::executeAudited('SELECT lastval() AS id'); + $row = $result->fetchRow(); + self::raiseExceptionOnError($row, 'fetching row for insertid failed'); return $row['id']; - } - if( $type == 'mssql' ) { + } else if( $type === 'mssql') { if($table !== null) { $prefix = OC_Config::getValue( "dbtableprefix", "oc_" ); $table = str_replace( '*PREFIX*', $prefix, $table ); } - return self::$connection->lastInsertId($table); - }else{ + $result = self::$connection->lastInsertId($table); + } else { if($table !== null) { $prefix = OC_Config::getValue( "dbtableprefix", "oc_" ); $suffix = OC_Config::getValue( "dbsequencesuffix", "_id_seq" ); $table = str_replace( '*PREFIX*', $prefix, $table ).$suffix; } - return self::$connection->lastInsertId($table); + $result = self::$connection->lastInsertId($table); } + self::raiseExceptionOnError($result, 'insertid failed'); + return $result; } /** @@ -512,6 +568,8 @@ class OC_DB { //clean up memory unlink( $file2 ); + + self::raiseExceptionOnError($definition,'Failed to parse the database definition'); // Die in case something went wrong if( $definition instanceof MDB2_Schema_Error ) { @@ -528,11 +586,7 @@ class OC_DB { $ret=self::$schema->createDatabase( $definition ); - // Die in case something went wrong - if( $ret instanceof MDB2_Error ) { - OC_Template::printErrorPage( self::$MDB2->getDebugOutput().' '.$ret->getMessage() . ': ' - . $ret->getUserInfo() ); - } + self::raiseExceptionOnError($ret,'Failed to create the database structure'); return true; } @@ -552,13 +606,7 @@ class OC_DB { $content = file_get_contents( $file ); $previousSchema = self::$schema->getDefinitionFromDatabase(); - if (PEAR::isError($previousSchema)) { - $error = $previousSchema->getMessage(); - $detail = $previousSchema->getDebugInfo(); - $message = 'Failed to get existing database structure for updating ('.$error.', '.$detail.')'; - OC_Log::write('core', $message, OC_Log::FATAL); - throw new Exception($message); - } + self::raiseExceptionOnError($previousSchema,'Failed to get existing database structure for updating'); // Make changes and save them to an in-memory file $file2 = 'static://db_scheme'; @@ -582,13 +630,7 @@ class OC_DB { //clean up memory unlink( $file2 ); - if (PEAR::isError($op)) { - $error = $op->getMessage(); - $detail = $op->getDebugInfo(); - $message = 'Failed to update database structure ('.$error.', '.$detail.')'; - OC_Log::write('core', $message, OC_Log::FATAL); - throw new Exception($message); - } + self::raiseExceptionOnError($op,'Failed to update database structure'); return true; } @@ -641,15 +683,9 @@ class OC_DB { } $query = substr($query, 0, strlen($query) - 5); try { - $stmt = self::prepare($query); - $result = $stmt->execute($inserts); - - } catch(PDOException $e) { - $entry = 'DB Error: "'.$e->getMessage() . '"
'; - $entry .= 'Offending command was: ' . $query . '
'; - OC_Log::write('core', $entry, OC_Log::FATAL); - error_log('DB error: '.$entry); - OC_Template::printErrorPage( $entry ); + $result = self::executeAudited($query, $inserts); + } catch(DatabaseException $e) { + OC_Template::printExceptionErrorPage( $e ); } if((int)$result->numRows() === 0) { @@ -674,16 +710,12 @@ class OC_DB { } try { - $result = self::prepare($query); + $result = self::executeAudited($query, $inserts); } catch(PDOException $e) { - $entry = 'DB Error: "'.$e->getMessage() . '"
'; - $entry .= 'Offending command was: ' . $query.'
'; - OC_Log::write('core', $entry, OC_Log::FATAL); - error_log('DB error: ' . $entry); - OC_Template::printErrorPage( $entry ); + OC_Template::printExceptionErrorPage( $e ); } - return $result->execute($inserts); + return $result; } /** @@ -891,7 +923,33 @@ class OC_DB { return false; } } + /** + * check if a result is an error, writes a log entry and throws an exception, works with MDB2 and PDOException + * @param mixed $result + * @param string message + * @return void + * @throws DatabaseException + */ + public static function raiseExceptionOnError($result, $message = null) { + if(self::isError($result)) { + if ($message === null) { + $message = self::getErrorMessage($result); + } else { + $message .= ', Root cause:' . self::getErrorMessage($result); + } + OC_Log::write('db', $message, OC_Log::FATAL); + throw new DatabaseException($message, getErrorCode($result)); + } + } + public static function getErrorCode($error) { + if ( self::$backend==self::BACKEND_MDB2 and PEAR::isError($error) ) { + $code = $error->getCode(); + } elseif ( self::$backend==self::BACKEND_PDO and self::$PDO ) { + $code = self::$PDO->errorCode(); + } + return $code; + } /** * returns the error code and message as a string for logging * works with MDB2 and PDOException @@ -901,9 +959,7 @@ class OC_DB { public static function getErrorMessage($error) { if ( self::$backend==self::BACKEND_MDB2 and PEAR::isError($error) ) { $msg = $error->getCode() . ': ' . $error->getMessage(); - if (defined('DEBUG') && DEBUG) { - $msg .= '(' . $error->getDebugInfo() . ')'; - } + $msg .= ' (' . $error->getDebugInfo() . ')'; } elseif (self::$backend==self::BACKEND_PDO and self::$PDO) { $msg = self::$PDO->errorCode() . ': '; $errorInfo = self::$PDO->errorInfo(); diff --git a/lib/template.php b/lib/template.php index 9467dedb62..01f0fc28b6 100644 --- a/lib/template.php +++ b/lib/template.php @@ -535,4 +535,25 @@ class OC_Template{ $content->printPage(); die(); } + + /** + * print error page using Exception details + * @param Exception $exception + */ + + public static function printExceptionErrorPage(Exception $exception) { + $error_msg = $exception->getMessage(); + if ($exception->getCode()) { + $error_msg = '['.$exception->getCode().'] '.$error_msg; + } + $hint = $exception->getTraceAsString(); + while ($exception = $exception->previous()) { + $error_msg .= '
Caused by: '; + if ($exception->getCode()) { + $error_msg .= '['.$exception->getCode().'] '; + } + $error_msg .= $exception->getMessage(); + }; + self::printErrorPage($error_msg, $hint); + } } From 8dc6bdd96b9088b87fe8d11346338343135c77e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 10 Jun 2013 13:45:19 +0200 Subject: [PATCH 2/3] clean up usage of DatabaseSetupException and catch Exceptions in entrypoints --- cron.php | 124 +++++++++++++++++++++++++---------------------- index.php | 13 ++++- lib/response.php | 4 ++ lib/setup.php | 24 ++++----- public.php | 42 ++++++++++------ remote.php | 77 ++++++++++++++++------------- status.php | 23 ++++++--- 7 files changed, 173 insertions(+), 134 deletions(-) diff --git a/cron.php b/cron.php index 95cedf8bf4..fbea7f26ae 100644 --- a/cron.php +++ b/cron.php @@ -44,75 +44,81 @@ function handleUnexpectedShutdown() { } } -require_once 'lib/base.php'; +try { -session_write_close(); + require_once 'lib/base.php'; -// Don't do anything if ownCloud has not been installed -if (!OC_Config::getValue('installed', false)) { - exit(0); -} + session_write_close(); -// Handle unexpected errors -register_shutdown_function('handleUnexpectedShutdown'); - -// Delete temp folder -OC_Helper::cleanTmpNoClean(); - -// Exit if background jobs are disabled! -$appmode = OC_BackgroundJob::getExecutionType(); -if ($appmode == 'none') { - TemporaryCronClass::$sent = true; - if (OC::$CLI) { - echo 'Background Jobs are disabled!' . PHP_EOL; - } else { - OC_JSON::error(array('data' => array('message' => 'Background jobs disabled!'))); - } - exit(1); -} - -if (OC::$CLI) { - // Create lock file first - TemporaryCronClass::$lockfile = OC_Config::getValue("datadirectory", OC::$SERVERROOT . '/data') . '/cron.lock'; - - // We call ownCloud from the CLI (aka cron) - if ($appmode != 'cron') { - // Use cron in feature! - OC_BackgroundJob::setExecutionType('cron'); + // Don't do anything if ownCloud has not been installed + if (!OC_Config::getValue('installed', false)) { + exit(0); } - // check if backgroundjobs is still running - if (file_exists(TemporaryCronClass::$lockfile)) { - TemporaryCronClass::$keeplock = true; + // Handle unexpected errors + register_shutdown_function('handleUnexpectedShutdown'); + + // Delete temp folder + OC_Helper::cleanTmpNoClean(); + + // Exit if background jobs are disabled! + $appmode = OC_BackgroundJob::getExecutionType(); + if ($appmode == 'none') { TemporaryCronClass::$sent = true; - echo "Another instance of cron.php is still running!"; + if (OC::$CLI) { + echo 'Background Jobs are disabled!' . PHP_EOL; + } else { + OC_JSON::error(array('data' => array('message' => 'Background jobs disabled!'))); + } exit(1); } - // Create a lock file - touch(TemporaryCronClass::$lockfile); + if (OC::$CLI) { + // Create lock file first + TemporaryCronClass::$lockfile = OC_Config::getValue("datadirectory", OC::$SERVERROOT . '/data') . '/cron.lock'; - // Work - $jobList = new \OC\BackgroundJob\JobList(); - $jobs = $jobList->getAll(); - foreach ($jobs as $job) { - $job->execute($jobList); - } -} else { - // We call cron.php from some website - if ($appmode == 'cron') { - // Cron is cron :-P - OC_JSON::error(array('data' => array('message' => 'Backgroundjobs are using system cron!'))); - } else { - // Work and success :-) + // We call ownCloud from the CLI (aka cron) + if ($appmode != 'cron') { + // Use cron in feature! + OC_BackgroundJob::setExecutionType('cron'); + } + + // check if backgroundjobs is still running + if (file_exists(TemporaryCronClass::$lockfile)) { + TemporaryCronClass::$keeplock = true; + TemporaryCronClass::$sent = true; + echo "Another instance of cron.php is still running!"; + exit(1); + } + + // Create a lock file + touch(TemporaryCronClass::$lockfile); + + // Work $jobList = new \OC\BackgroundJob\JobList(); - $job = $jobList->getNext(); - $job->execute($jobList); - $jobList->setLastJob($job); - OC_JSON::success(); + $jobs = $jobList->getAll(); + foreach ($jobs as $job) { + $job->execute($jobList); + } + } else { + // We call cron.php from some website + if ($appmode == 'cron') { + // Cron is cron :-P + OC_JSON::error(array('data' => array('message' => 'Backgroundjobs are using system cron!'))); + } else { + // Work and success :-) + $jobList = new \OC\BackgroundJob\JobList(); + $job = $jobList->getNext(); + $job->execute($jobList); + $jobList->setLastJob($job); + OC_JSON::success(); + } } -} -// done! -TemporaryCronClass::$sent = true; -exit(); + // done! + TemporaryCronClass::$sent = true; + exit(); + +} catch (Exception $ex) { + \OCP\Util::writeLog('cron', $ex->getMessage(), \OCP\Util::FATAL); +} \ No newline at end of file diff --git a/index.php b/index.php index bf0b287a64..a064aa5c76 100755 --- a/index.php +++ b/index.php @@ -23,6 +23,15 @@ $RUNTIME_NOAPPS = true; //no apps, yet -require_once 'lib/base.php'; +try { + + require_once 'lib/base.php'; -OC::handleRequest(); + OC::handleRequest(); + +} catch (Exception $ex) { + //show the user a detailed error page + OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); + \OCP\Util::writeLog('remote', $ex->getMessage(), \OCP\Util::FATAL); + OC_Template::printExceptionErrorPage($ex); +} \ No newline at end of file diff --git a/lib/response.php b/lib/response.php index 49d79fda70..674176d078 100644 --- a/lib/response.php +++ b/lib/response.php @@ -11,6 +11,7 @@ class OC_Response { const STATUS_NOT_MODIFIED = 304; const STATUS_TEMPORARY_REDIRECT = 307; const STATUS_NOT_FOUND = 404; + const STATUS_INTERNAL_SERVER_ERROR = 500; /** * @brief Enable response caching by sending correct HTTP headers @@ -70,6 +71,9 @@ class OC_Response { case self::STATUS_NOT_FOUND; $status = $status . ' Not Found'; break; + case self::STATUS_INTERNAL_SERVER_ERROR; + $status = $status . ' Internal Server Error'; + break; } header($protocol.' '.$status); } diff --git a/lib/setup.php b/lib/setup.php index a63cc664db..71a2d13937 100644 --- a/lib/setup.php +++ b/lib/setup.php @@ -106,12 +106,6 @@ class OC_Setup { 'hint' => $e->getHint() ); return($error); - } catch (Exception $e) { - $error[] = array( - 'error' => $e->getMessage(), - 'hint' => '' - ); - return($error); } } elseif($dbtype == 'pgsql') { @@ -127,7 +121,7 @@ class OC_Setup { try { self::setupPostgreSQLDatabase($dbhost, $dbuser, $dbpass, $dbname, $dbtableprefix, $username); - } catch (Exception $e) { + } catch (DatabaseSetupException $e) { $error[] = array( 'error' => $l->t('PostgreSQL username and/or password not valid'), 'hint' => $l->t('You need to enter either an existing account or the administrator.') @@ -150,7 +144,7 @@ class OC_Setup { try { self::setupOCIDatabase($dbhost, $dbuser, $dbpass, $dbname, $dbtableprefix, $dbtablespace, $username); - } catch (Exception $e) { + } catch (DatabaseSetupException $e) { $error[] = array( 'error' => $l->t('Oracle connection could not be established'), 'hint' => $e->getMessage().' Check environment: ORACLE_HOME='.getenv('ORACLE_HOME') @@ -177,7 +171,7 @@ class OC_Setup { try { self::setupMSSQLDatabase($dbhost, $dbuser, $dbpass, $dbname, $dbtableprefix); - } catch (Exception $e) { + } catch (DatabaseSetupException $e) { $error[] = array( 'error' => 'MS SQL username and/or password not valid', 'hint' => 'You need to enter either an existing account or the administrator.' @@ -326,7 +320,7 @@ class OC_Setup { $connection_string = "host='$e_host' dbname=postgres user='$e_user' password='$e_password'"; $connection = @pg_connect($connection_string); if(!$connection) { - throw new Exception($l->t('PostgreSQL username and/or password not valid')); + throw new DatabaseSetupException($l->t('PostgreSQL username and/or password not valid')); } $e_user = pg_escape_string($dbuser); //check for roles creation rights in postgresql @@ -371,7 +365,7 @@ class OC_Setup { $connection_string = "host='$e_host' dbname='$e_dbname' user='$e_user' password='$e_password'"; $connection = @pg_connect($connection_string); if(!$connection) { - throw new Exception($l->t('PostgreSQL username and/or password not valid')); + throw new DatabaseSetupException($l->t('PostgreSQL username and/or password not valid')); } $query = "select count(*) FROM pg_class WHERE relname='{$dbtableprefix}users' limit 1"; $result = pg_query($connection, $query); @@ -461,9 +455,9 @@ class OC_Setup { if(!$connection) { $e = oci_error(); if (is_array ($e) && isset ($e['message'])) { - throw new Exception($e['message']); + throw new DatabaseSetupException($e['message']); } - throw new Exception($l->t('Oracle username and/or password not valid')); + throw new DatabaseSetupException($l->t('Oracle username and/or password not valid')); } //check for roles creation rights in oracle @@ -530,7 +524,7 @@ class OC_Setup { } $connection = @oci_connect($dbuser, $dbpass, $easy_connect_string); if(!$connection) { - throw new Exception($l->t('Oracle username and/or password not valid')); + throw new DatabaseSetupException($l->t('Oracle username and/or password not valid')); } $query = "SELECT count(*) FROM user_tables WHERE table_name = :un"; $stmt = oci_parse($connection, $query); @@ -641,7 +635,7 @@ class OC_Setup { } else { $entry = ''; } - throw new Exception($l->t('MS SQL username and/or password not valid: %s', array($entry))); + throw new DatabaseSetupException($l->t('MS SQL username and/or password not valid: %s', array($entry))); } OC_Config::setValue('dbuser', $dbuser); diff --git a/public.php b/public.php index 3d7fd378af..0154b59cce 100644 --- a/public.php +++ b/public.php @@ -1,21 +1,31 @@ getMessage(), \OCP\Util::FATAL); + OC_Template::printExceptionErrorPage($ex); +} \ No newline at end of file diff --git a/remote.php b/remote.php index 7738de04f6..ec0f2ecef7 100644 --- a/remote.php +++ b/remote.php @@ -1,40 +1,49 @@ getMessage(), \OCP\Util::FATAL); + OC_Template::printExceptionErrorPage($ex); +} \ No newline at end of file diff --git a/status.php b/status.php index 9d6ac87c67..bac01c11b2 100644 --- a/status.php +++ b/status.php @@ -23,13 +23,20 @@ $RUNTIME_NOAPPS = true; //no apps, yet -require_once 'lib/base.php'; +try { -if(OC_Config::getValue('installed')==1) $installed='true'; else $installed='false'; -$values=array( - 'installed'=>$installed, - 'version'=>implode('.', OC_Util::getVersion()), - 'versionstring'=>OC_Util::getVersionString(), - 'edition'=>OC_Util::getEditionString()); + require_once 'lib/base.php'; -echo(json_encode($values)); + if(OC_Config::getValue('installed')==1) $installed='true'; else $installed='false'; + $values=array( + 'installed'=>$installed, + 'version'=>implode('.', OC_Util::getVersion()), + 'versionstring'=>OC_Util::getVersionString(), + 'edition'=>OC_Util::getEditionString()); + + echo(json_encode($values)); + +} catch (Exception $ex) { + OC_Response::setStatus(OC_Response::STATUS_INTERNAL_SERVER_ERROR); + \OCP\Util::writeLog('remote', $ex->getMessage(), \OCP\Util::FATAL); +} \ No newline at end of file From 86c5243be5525b97b232b23c241a60524455a41c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B6rn=20Friedrich=20Dreyer?= Date: Mon, 10 Jun 2013 14:03:12 +0200 Subject: [PATCH 3/3] remove duplicate logging from db.php, now happens in entrypoints --- lib/db.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/lib/db.php b/lib/db.php index ccbfcf3ab5..080e895e35 100644 --- a/lib/db.php +++ b/lib/db.php @@ -408,7 +408,6 @@ class OC_DB { // TODO try to convert LIMIT OFFSET notation to parameters, see fixLimitClauseForMSSQL $message = 'LIMIT and OFFSET are forbidden for portability reasons,' . ' pass an array with \'limit\' and \'offset\' instead'; - \OCP\Util::writeLog('db', $message, \OCP\Util::FATAL); throw new DatabaseException($message); } $stmt = array('sql' => $stmt, 'limit' => null, 'offset' => null); @@ -417,7 +416,6 @@ class OC_DB { // convert to prepared statement if ( ! array_key_exists('sql', $stmt) ) { $message = 'statement array must at least contain key \'sql\''; - \OCP\Util::writeLog('db', $message, \OCP\Util::FATAL); throw new DatabaseException($message); } if ( ! array_key_exists('limit', $stmt) ) { @@ -438,7 +436,6 @@ class OC_DB { } else { $message = 'Expected a prepared statement or array got ' . gettype($stmt); } - \OCP\Util::writeLog('db', $message, \OCP\Util::FATAL); throw new DatabaseException($message); } return $result; @@ -937,7 +934,6 @@ class OC_DB { } else { $message .= ', Root cause:' . self::getErrorMessage($result); } - OC_Log::write('db', $message, OC_Log::FATAL); throw new DatabaseException($message, getErrorCode($result)); } }