You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2015/01/20 15:12:22 UTC
svn commit: r1653250 - /subversion/trunk/subversion/libsvn_subr/sqlite.c
Author: philip
Date: Tue Jan 20 14:12:21 2015
New Revision: 1653250
URL: http://svn.apache.org/r1653250
Log:
Fix a memory leak when svn_sqlite__open returns an error after opening
the SQLite database. SQLite always requires sqlite3_close after
sqlite3_open_v2 while Subversion typically does not require close after
an open that fails. This fixes a memory leak in wc_tests.py 8 after
failing to open a corrupt wc.db.
* subversion/libsvn_subr/sqlite.c
(SQLITE_ERR_CLOSE, SVN_ERR_CLOSE): New macros.
(internal_open): Change parameter to svn_sqlite__db_t*, use new
macros to always close db on error.
(close_apr): Combine 2 loops into 1, check prepated_stmts is allocated
before looping to allow closing when an opening error occurs.
(svn_sqlite__open): Use new macros to always close db on error.
Modified:
subversion/trunk/subversion/libsvn_subr/sqlite.c
Modified: subversion/trunk/subversion/libsvn_subr/sqlite.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/sqlite.c?rev=1653250&r1=1653249&r2=1653250&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/sqlite.c (original)
+++ subversion/trunk/subversion/libsvn_subr/sqlite.c Tue Jan 20 14:12:21 2015
@@ -189,6 +189,21 @@ struct svn_sqlite__value_t
sqlite3_errmsg((db)->db3)); \
} while (0)
+#define SQLITE_ERR_CLOSE(x, db, pool) do \
+{ \
+ int sqlite_err__temp = (x); \
+ if (sqlite_err__temp != SQLITE_OK) \
+ { \
+ const char *sqlite_err__msg \
+ = apr_pstrdup(pool, sqlite3_errmsg((db)->db3)); \
+ return svn_error_compose_create( \
+ svn_error_createf(SQLITE_ERROR_CODE(sqlite_err__temp), \
+ NULL, "sqlite[S%d]: %s", \
+ sqlite_err__temp, sqlite_err__msg), \
+ svn_sqlite__close(db)); \
+ } \
+} while (0)
+
#define SQLITE_ERR_MSG(x, msg) do \
{ \
int sqlite_err__temp = (x); \
@@ -198,6 +213,13 @@ struct svn_sqlite__value_t
sqlite_err__temp, msg); \
} while (0)
+#define SVN_ERR_CLOSE(x, db) do \
+{ \
+ svn_error_t *svn__err = (x); \
+ if (svn__err) \
+ return svn_error_compose_create(svn__err, svn_sqlite__close(db)); \
+} while (0)
+
/* Time (in milliseconds) to wait for sqlite locks before giving up. */
#define BUSY_TIMEOUT 10000
@@ -809,7 +831,7 @@ init_sqlite(void *baton, apr_pool_t *poo
}
static svn_error_t *
-internal_open(sqlite3 **db3, const char *path, svn_sqlite__mode_t mode,
+internal_open(svn_sqlite__db_t *db, const char *path, svn_sqlite__mode_t mode,
apr_int32_t timeout, apr_pool_t *scratch_pool)
{
{
@@ -856,13 +878,13 @@ internal_open(sqlite3 **db3, const char
/* Open the database. Note that a handle is returned, even when an error
occurs (except for out-of-memory); thus, we can safely use it to
- extract an error message and construct an svn_error_t. */
+ extract an error message and construct an svn_error_t. SQLite always
+ requires sqlite3_close() after sqlite3_open_v2() while Subversion
+ typically does not require close() after an open() that returns an
+ error. So we must ensure we close the handle if this function, or
+ the caller svn_sqlite__open, returns an error to the application. */
{
const char *vFs = NULL;
- int err_code;
- /* We'd like to use SQLITE_ERR here, but we can't since it would
- just return an error and leave the database open. So, we need to
- do this manually. */
#if defined(WIN32) && SQLITE_VERSION_AT_LEAST(3, 8, 1)
if (strlen(path) > 248)
@@ -886,18 +908,8 @@ internal_open(sqlite3 **db3, const char
#endif
/* ### SQLITE_CANTOPEN */
- err_code = sqlite3_open_v2(path, db3, flags, vFs);
- if (err_code != SQLITE_OK)
- {
- /* Save the error message before closing the SQLite handle. */
- char *msg = apr_pstrdup(scratch_pool, sqlite3_errmsg(*db3));
-
- /* We don't catch the error here, since we care more about the open
- error than the close error at this point. */
- sqlite3_close(*db3);
-
- SQLITE_ERR_MSG(err_code, msg);
- }
+ SQLITE_ERR_CLOSE(sqlite3_open_v2(path, &db->db3, flags, vFs),
+ db, scratch_pool);
}
}
@@ -905,7 +917,8 @@ internal_open(sqlite3 **db3, const char
timeout = BUSY_TIMEOUT;
/* Retry until timeout when database is busy. */
- SQLITE_ERR_MSG(sqlite3_busy_timeout(*db3, timeout), sqlite3_errmsg(*db3));
+ SQLITE_ERR_CLOSE(sqlite3_busy_timeout(db->db3, timeout),
+ db, scratch_pool);
return SVN_NO_ERROR;
}
@@ -925,35 +938,29 @@ close_apr(void *data)
if (db->db3 == NULL)
return APR_SUCCESS;
- /* Finalize any existing prepared statements. */
- for (i = 0; i < db->nbr_statements; i++)
+ /* Finalize any prepared statements. */
+ if (db->prepared_stmts)
{
- if (db->prepared_stmts[i])
+ for (i = 0; i < db->nbr_statements + STMT_INTERNAL_LAST; i++)
{
- if (db->prepared_stmts[i]->needs_reset)
+ if (db->prepared_stmts[i])
{
+ if (i < db->nbr_statements
+ && db->prepared_stmts[i]->needs_reset)
+ {
#ifdef SVN_DEBUG
- const char *stmt_text = db->statement_strings[i];
- SVN_UNUSED(stmt_text);
+ const char *stmt_text = db->statement_strings[i];
+ SVN_UNUSED(stmt_text);
- SVN_ERR_MALFUNCTION_NO_RETURN();
+ SVN_ERR_MALFUNCTION_NO_RETURN();
#else
- err = svn_error_compose_create(
- err,
+ err = svn_error_compose_create(err,
svn_sqlite__reset(db->prepared_stmts[i]));
#endif
- }
- err = svn_error_compose_create(
- svn_sqlite__finalize(db->prepared_stmts[i]), err);
- }
- }
- /* And finalize any used internal statements */
- for (; i < db->nbr_statements + STMT_INTERNAL_LAST; i++)
- {
- if (db->prepared_stmts[i])
- {
- err = svn_error_compose_create(
+ }
+ err = svn_error_compose_create(
svn_sqlite__finalize(db->prepared_stmts[i]), err);
+ }
}
}
@@ -1081,7 +1088,7 @@ svn_sqlite__open(svn_sqlite__db_t **db,
*db = apr_pcalloc(result_pool, sizeof(**db));
- SVN_ERR(internal_open(&(*db)->db3, path, mode, timeout, scratch_pool));
+ SVN_ERR(internal_open(*db, path, mode, timeout, scratch_pool));
#if SQLITE_VERSION_NUMBER >= 3008000 && SQLITE_VERSION_NUMBER < 3009000
/* disable SQLITE_ENABLE_STAT3/4 from 3.8.1 - 3.8.3 (but not 3.8.3.1+)
@@ -1102,10 +1109,10 @@ svn_sqlite__open(svn_sqlite__db_t **db,
svn_membuf__create(&(*db)->sqlext_buf3, 800, result_pool);
/* Register collation and LIKE and GLOB operator replacements. */
- SQLITE_ERR(sqlite3_create_collation((*db)->db3,
- "svn-ucs-nfd", SQLITE_UTF8,
- *db, collate_ucs_nfd),
- *db);
+ SQLITE_ERR_CLOSE(sqlite3_create_collation((*db)->db3,
+ "svn-ucs-nfd", SQLITE_UTF8,
+ *db, collate_ucs_nfd),
+ db, scratch_pool);
/* ### Is it really necessary to override these functions?
I would assume the default implementation to be collation agnostic?
And otherwise our implementation should be...
@@ -1113,18 +1120,18 @@ svn_sqlite__open(svn_sqlite__db_t **db,
The default implementation is in some cases index backed, while our
implementation can't be. With an index based on the collation it could
be. */
- SQLITE_ERR(sqlite3_create_function((*db)->db3, "glob", 2,
- SQLITE_UTF8 | SQLITE_DETERMINISTIC,
- *db, glob_ucs_nfd, NULL, NULL),
- *db);
- SQLITE_ERR(sqlite3_create_function((*db)->db3, "like", 2,
- SQLITE_UTF8 | SQLITE_DETERMINISTIC,
- *db, like_ucs_nfd, NULL, NULL),
- *db);
- SQLITE_ERR(sqlite3_create_function((*db)->db3, "like", 3,
- SQLITE_UTF8 | SQLITE_DETERMINISTIC,
- *db, like_ucs_nfd, NULL, NULL),
- *db);
+ SQLITE_ERR_CLOSE(sqlite3_create_function((*db)->db3, "glob", 2,
+ SQLITE_UTF8 | SQLITE_DETERMINISTIC,
+ *db, glob_ucs_nfd, NULL, NULL),
+ db, scratch_pool);
+ SQLITE_ERR_CLOSE(sqlite3_create_function((*db)->db3, "like", 2,
+ SQLITE_UTF8 | SQLITE_DETERMINISTIC,
+ *db, like_ucs_nfd, NULL, NULL),
+ db, scratch_pool);
+ SQLITE_ERR_CLOSE(sqlite3_create_function((*db)->db3, "like", 3,
+ SQLITE_UTF8 | SQLITE_DETERMINISTIC,
+ *db, like_ucs_nfd, NULL, NULL),
+ db, scratch_pool);
#endif /* SVN_UNICODE_NORMALIZATION_FIXES */
#ifdef SQLITE3_DEBUG
@@ -1134,7 +1141,7 @@ svn_sqlite__open(svn_sqlite__db_t **db,
sqlite3_profile((*db)->db3, sqlite_profiler, (*db)->db3);
#endif
- SVN_ERR(exec_sql(*db,
+ SVN_ERR_CLOSE(exec_sql(*db,
/* The default behavior of the LIKE operator is to ignore case
for ASCII characters. Hence, by default 'a' LIKE 'A' is true.
The case_sensitive_like pragma installs a new application-
@@ -1164,13 +1171,15 @@ svn_sqlite__open(svn_sqlite__db_t **db,
affects application(read: Subversion) performance/behavior. */
"PRAGMA foreign_keys=OFF;" /* SQLITE_DEFAULT_FOREIGN_KEYS*/
"PRAGMA locking_mode = NORMAL;" /* SQLITE_DEFAULT_LOCKING_MODE */
- ));
+ ),
+ *db);
#if defined(SVN_DEBUG)
/* When running in debug mode, enable the checking of foreign key
constraints. This has possible performance implications, so we don't
bother to do it for production...for now. */
- SVN_ERR(exec_sql(*db, "PRAGMA foreign_keys=ON;"));
+ SVN_ERR_CLOSE(exec_sql(*db, "PRAGMA foreign_keys=ON;"),
+ *db);
#endif
#ifdef SVN_SQLITE_REVERSE_UNORDERED_SELECTS
@@ -1178,7 +1187,8 @@ svn_sqlite__open(svn_sqlite__db_t **db,
clause to emit their results in the reverse order of what they normally
would. This can help detecting invalid assumptions about the result
order.*/
- SVN_ERR(exec_sql(*db, "PRAGMA reverse_unordered_selects=ON;"));
+ SVN_ERR_CLOSE(exec_sql(*db, "PRAGMA reverse_unordered_selects=ON;"),
+ *db);
#endif
/* Store temporary tables in RAM instead of in temporary files, but don't