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 <mrylander@gmail.com>

diff --git a/Open-ILS/src/c-apps/oils_sql.c b/Open-ILS/src/c-apps/oils_sql.c
index d8afb6d..24072c6 100644 (file)
--- 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 );