From: Mike Rylander Date: Thu, 20 Feb 2020 15:43:22 +0000 (-0500) Subject: LP#1847805: Use WITHOUT HOLD cursor X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=016009f63448e9f724ce619a0fd3c775acd9d144;p=working%2FEvergreen.git LP#1847805: Use WITHOUT HOLD cursor If we're not inside an explicit transaction, we start one ourselves so that we can use WITHOUT HOLD cursors. Unfortunately, WITH HOLD cursors can create large temp files which may be an attack vector. The commit also includes leak avoidance for the cursor_name string. It is now a single global variable, which is safe in the current code because fleshing sub-calls occur after the full set of results are retrieved at the current level, allowing any implicit transaction to be closed. NOTE: Using cursors could allow us to interleve fleshing sub-calls which could allow a faster TTFB, but cursor names would need to live on the stack rather than be the new single global. Signed-off-by: Mike Rylander --- diff --git a/Open-ILS/src/c-apps/oils_sql.c b/Open-ILS/src/c-apps/oils_sql.c index d8afb6dd38..24072c6128 100644 --- a/Open-ILS/src/c-apps/oils_sql.c +++ b/Open-ILS/src/c-apps/oils_sql.c @@ -74,6 +74,7 @@ static void setXactId( osrfMethodContext* ctx ); static inline const char* getXactId( osrfMethodContext* ctx ); static inline void clearXactId( osrfMethodContext* ctx ); +static char* free_cursor_name(); static char* random_cursor_name(); static jsonObject* doFieldmapperSearch ( osrfMethodContext* ctx, osrfHash* class_meta, jsonObject* where_hash, jsonObject* query_hash, int* err ); @@ -93,7 +94,7 @@ static char* searchPredicate ( const ClassInfo*, osrfHash*, jsonObject*, osrfMet static char* searchJOIN ( const jsonObject*, const ClassInfo* left_info ); static char* searchWHERE ( const jsonObject* search_hash, const ClassInfo*, int, osrfMethodContext* ); static char* buildSELECT( const jsonObject*, jsonObject* rest_of_query, - osrfHash* meta, osrfMethodContext* ctx, char* cursor_name ); + osrfHash* meta, osrfMethodContext* ctx ); static char* buildOrderByFromArray( osrfMethodContext* ctx, const jsonObject* order_array ); char* buildQuery( osrfMethodContext* ctx, jsonObject* query, int flags ); @@ -150,7 +151,13 @@ int writeAuditInfo( osrfMethodContext* ctx, const char* user_id, const char* ws_ static char* _sanitize_tz_name( const char* tz ); static char* _sanitize_savepoint_name( const char* sp ); +static char* cursor_name = NULL; const char charset[] = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_"; +static char* free_cursor_name() { + if (cursor_name) free(cursor_name); + cursor_name = NULL; + return cursor_name; +} static char* random_cursor_name() { char* str = safe_malloc(sizeof(char) * (ALIAS_STORE_SIZE + 1)); // 16 character cursor name + \0 int n; @@ -5492,7 +5499,7 @@ static char* buildOrderByFromArray( osrfMethodContext* ctx, const jsonObject* or The SELECT statements built here are distinct from those built for the json_query method. */ static char* buildSELECT ( const jsonObject* search_hash, jsonObject* rest_of_query, - osrfHash* meta, osrfMethodContext* ctx, char* cursor_name ) { + osrfHash* meta, osrfMethodContext* ctx ) { const char* locale = osrf_message_get_last_locale(); @@ -5607,7 +5614,7 @@ static char* buildSELECT ( const jsonObject* search_hash, jsonObject* rest_of_qu if( !table ) table = strdup( "(null)" ); - if (cursor_name) buffer_fadd( sql_buf, "DECLARE \"%s\" NO SCROLL CURSOR WITH HOLD FOR ", cursor_name ); + if (cursor_name) buffer_fadd( sql_buf, "DECLARE \"%s\" NO SCROLL CURSOR WITHOUT HOLD FOR ", cursor_name ); buffer_fadd( sql_buf, "SELECT %s FROM %s AS \"%s\"", col_list, table, core_class ); free( col_list ); free( table ); @@ -5962,15 +5969,15 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ int flesh_depth = 0; int offset = 0; int limit = 0; - int cursor_page_size = 5; - char* cursor_name = NULL; + int cursor_page_size = 50; if (*methodtype != 'r') cursor_name = random_cursor_name(); /* Use a cursor for all but .retrieve */ - char* sql = buildSELECT( where_hash, query_hash, class_meta, ctx, cursor_name ); + char* sql = buildSELECT( where_hash, query_hash, class_meta, ctx ); if( !sql ) { osrfLogDebug( OSRF_LOG_MARK, "Problem building query, returning NULL" ); *err = -1; + free_cursor_name(); return NULL; } @@ -5988,6 +5995,7 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ "osrfMethodException", ctx->request, "Error setting timezone" ); if( !oilsIsDBConnected( writehandle )) { osrfAppSessionPanic( ctx->session ); + free_cursor_name(); return NULL; } } else { @@ -6001,6 +6009,7 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ osrfLogError( OSRF_LOG_MARK, "%s: Error resetting timezone", modulename); if( !oilsIsDBConnected( writehandle )) { osrfAppSessionPanic( ctx->session ); + free_cursor_name(); return NULL; } } else { @@ -6028,9 +6037,29 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ // set cursor_page_size to the largest of the three if (cursor_page_size < offset) cursor_page_size = offset; if (cursor_page_size < limit) cursor_page_size = limit; + + if (!getXactId(ctx)) { + // Start a transaction for the cursor, if not inside an explicit one, + // so we can use WITHOUT HOLD and avoid temp file attacks. + dbi_result result = dbi_conn_query( writehandle, "START TRANSACTION;" ); + if( !result ) { + free_cursor_name(); + const char* msg; + int errnum = dbi_conn_error( writehandle, &msg ); + osrfLogError( OSRF_LOG_MARK, "%s: Error starting transaction for cursor: %d %s", + modulename, errnum, msg ? msg : "(No description available)" ); + osrfAppSessionStatus( ctx->session, OSRF_STATUS_INTERNALSERVERERROR, + "osrfMethodException", ctx->request, "Error starting transaction for cursor" ); + if( !oilsIsDBConnected( writehandle )) + osrfAppSessionPanic( ctx->session ); + return NULL; + } else { + dbi_result_free( result ); + } + } } - // Declares a cursor or runs a retrieve-select + // Declares a cursor or runs a retrieve-select dbi_result result = dbi_conn_query( dbhandle, sql ); if( NULL == result ) { @@ -6050,6 +6079,11 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ ); *err = -1; free( sql ); + if (cursor_name) { // we're inside a transaction for sure, but dead in the water -- try to rollback! + result = dbi_conn_query( writehandle, "ROLLBACK;" ); + if (result) dbi_result_free(result); + } + free_cursor_name(); return NULL; } else { @@ -6108,6 +6142,7 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ ); *err = -1; free( sql ); + free_cursor_name(); return NULL; } @@ -6167,6 +6202,7 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ ); *err = -1; free( sql ); + free_cursor_name(); return NULL; } } @@ -6190,6 +6226,28 @@ static jsonObject* doFieldmapperSearch( osrfMethodContext* ctx, osrfHash* class_ if (cursor_name) { result = dbi_conn_queryf( dbhandle, "CLOSE \"%s\";", cursor_name ); dbi_result_free( result ); + free_cursor_name(); // release the cursor name now so recursive fleshing calls can make their own without leaking + + // We magically create a transaction for cursors when /not/ in an explicit one, + // so failing this test means no explicit xact exists, thus there must be an + // implicit one. + if (!getXactId(ctx)) { + // We only use a cursor xact in retrieve-ish methods, which are read-only, so roll back rather than commit. + result = dbi_conn_query( writehandle, "ROLLBACK;" ); + if( !result ) { + const char* msg; + int errnum = dbi_conn_error( writehandle, &msg ); + osrfLogError( OSRF_LOG_MARK, "%s: Error rolling back transaction for cursor: %d %s", + modulename, errnum, msg ? msg : "(No description available)" ); + osrfAppSessionStatus( ctx->session, OSRF_STATUS_INTERNALSERVERERROR, + "osrfMethodException", ctx->request, "Error rolling back transaction for cursor" ); + if( !oilsIsDBConnected( writehandle )) + osrfAppSessionPanic( ctx->session ); + return NULL; + } else { + dbi_result_free( result ); + } + } } free( sql );