From 93135025e76e0b42e93dba4eadf1023295576059 Mon Sep 17 00:00:00 2001 From: scottmk Date: Tue, 25 May 2010 16:21:56 +0000 Subject: [PATCH] Plug some memory leaks, and eliminate some unnecessary memory allocations. In oils_utils.[ch]: -- Create a new function, oilsFMGetStringConst(), similar to oilsFMGetString() except that it doesn't allocate memory; it returns a const pointer to a string internal to the specified object. -- Add some comments, tidy up white space. In oils_sql.c: -- Replace oilsFMGetString() with oilsFMGetStringConst in a number of places; partly to reduce memory churn, and partly to plug some memory leaks where the function call was nested within a parameter list. -- Change org_tree_root() so as to return a const pointer to a static buffer (which was already in use as a cache) instead of allocating a copy of the string. This change reduces memory churn. In addition the allocated string was leaking anyway, and now that leak is plugged. M Open-ILS/include/openils/oils_utils.h M Open-ILS/src/c-apps/oils_sql.c M Open-ILS/src/c-apps/oils_utils.c git-svn-id: svn://svn.open-ils.org/ILS/trunk@16494 dcc99617-32d9-48b4-a31d-7c20da2025e4 --- Open-ILS/include/openils/oils_utils.h | 20 +-- Open-ILS/src/c-apps/oils_sql.c | 32 ++--- Open-ILS/src/c-apps/oils_utils.c | 171 ++++++++++++++++++++++---- 3 files changed, 159 insertions(+), 64 deletions(-) diff --git a/Open-ILS/include/openils/oils_utils.h b/Open-ILS/include/openils/oils_utils.h index 3c56103333..fae7e4ddfe 100644 --- a/Open-ILS/include/openils/oils_utils.h +++ b/Open-ILS/include/openils/oils_utils.h @@ -26,26 +26,10 @@ extern "C" { */ osrfHash* oilsInitIDL( const char* idl_filename ); -/** - Returns the string value for field 'field' in the given object. - This method calls jsonObjectToSimpleString so numbers will be - returned as strings. - @param object The object to inspect - @param field The field whose value is requsted - @return The string at the given position, if none exists, - then NULL is returned. The caller must free the returned string - */ -char* oilsFMGetString( const jsonObject* object, const char* field ); +const char* oilsFMGetStringConst( const jsonObject* object, const char* field ); +char* oilsFMGetString( const jsonObject* object, const char* field ); -/** - Returns the jsonObject found at the specified field in the - given object. - @param object The object to inspect - @param field The field whose value is requsted - @return The found object or NULL if none exists. Do NOT free the - returned object. - */ const jsonObject* oilsFMGetObject( const jsonObject* object, const char* field ); /** diff --git a/Open-ILS/src/c-apps/oils_sql.c b/Open-ILS/src/c-apps/oils_sql.c index c23bb9426c..f87483f9eb 100644 --- a/Open-ILS/src/c-apps/oils_sql.c +++ b/Open-ILS/src/c-apps/oils_sql.c @@ -114,7 +114,7 @@ static void clear_query_stack( void ); static const jsonObject* verifyUserPCRUD( osrfMethodContext* ); static int verifyObjectPCRUD( osrfMethodContext*, const jsonObject* ); -static char* org_tree_root( osrfMethodContext* ctx ); +static const char* org_tree_root( osrfMethodContext* ctx ); static jsonObject* single_hash( const char* key, const char* value ); static int child_initialized = 0; /* boolean */ @@ -1302,7 +1302,7 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) if( !user ) return 0; // Not logged in? No access. - int userid = atoi( oilsFMGetString( user, "id" ) ); + int userid = atoi( oilsFMGetStringConst( user, "id" ) ); // Get a list of permissions from the permacrud entry. osrfStringArray* permission = osrfHashGet( pcrud, "permission" ); @@ -1325,7 +1325,7 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) osrfStringArray* context_org_array = osrfNewStringArray( 1 ); int err = 0; - char* pkey_value = NULL; + const char* pkey_value = NULL; if( str_is_true( osrfHashGet(pcrud, "global_required") ) ) { // If the global_required attribute is present and true, then the only owning // org unit is the root org unit, i.e. the one with no parent. @@ -1333,7 +1333,7 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) "global-level permissions required, fetching top of the org tree" ); // check for perm at top of org tree - char* org_tree_root_id = org_tree_root( ctx ); + const char* org_tree_root_id = org_tree_root( ctx ); if( org_tree_root_id ) { osrfStringArrayAdd( context_org_array, org_tree_root_id ); osrfLogDebug( OSRF_LOG_MARK, "top of the org tree is %s", org_tree_root_id ); @@ -1357,13 +1357,13 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) jsonObject *param = NULL; if( obj->classname ) { - pkey_value = oilsFMGetString( obj, pkey ); + pkey_value = oilsFMGetStringConst( obj, pkey ); if( !fetch ) param = jsonObjectClone( obj ); osrfLogDebug( OSRF_LOG_MARK, "Object supplied, using primary key value of %s", pkey_value ); } else { - pkey_value = jsonObjectToSimpleString( obj ); + pkey_value = jsonObjectGetString( obj ); fetch = 1; osrfLogDebug( OSRF_LOG_MARK, "Object not supplied, using primary key value " "of %s and retrieving from the database", pkey_value ); @@ -1404,9 +1404,6 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) ); free( m ); - if( pkey_value ) - free( pkey_value ); - return 0; } @@ -1419,7 +1416,7 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) int i = 0; const char* lcontext = NULL; while ( (lcontext = osrfStringArrayGetString(local_context, i++)) ) { - osrfStringArrayAdd( context_org_array, oilsFMGetString( param, lcontext ) ); + osrfStringArrayAdd( context_org_array, oilsFMGetStringConst( param, lcontext )); osrfLogDebug( OSRF_LOG_MARK, "adding class-local field %s (value: %s) to the context org list", @@ -1548,7 +1545,7 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) osrfStringArray* ctx_array = osrfHashGet( fcontext, "context" ); while ( (foreign_field = osrfStringArrayGetString( ctx_array, j++ )) ) { osrfStringArrayAdd( context_org_array, - oilsFMGetString( _fparam, foreign_field ) ); + oilsFMGetStringConst( _fparam, foreign_field )); osrfLogDebug( OSRF_LOG_MARK, "adding foreign class %s field %s (value: %s) to the context org list", @@ -1584,7 +1581,7 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) while( (context_org = osrfStringArrayGetString( context_org_array, j++ )) ) { dbi_result result; - if( pkey_value ) { + if( pkey_value ) { osrfLogDebug( OSRF_LOG_MARK, "Checking object permission [%s] for user %d " @@ -1683,8 +1680,6 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) break; } - if( pkey_value ) - free(pkey_value); osrfStringArrayFree( context_org_array ); return OK; @@ -1702,7 +1697,7 @@ static int verifyObjectPCRUD ( osrfMethodContext* ctx, const jsonObject* obj ) The calling code is responsible for freeing the returned string. */ -static char* org_tree_root( osrfMethodContext* ctx ) { +static const char* org_tree_root( osrfMethodContext* ctx ) { static char cached_root_id[ 32 ] = ""; // extravagantly large buffer static time_t last_lookup_time = 0; @@ -1740,13 +1735,12 @@ static char* org_tree_root( osrfMethodContext* ctx ) { return NULL; } - char* root_org_unit_id = oilsFMGetString( tree_top, "id" ); + const char* root_org_unit_id = oilsFMGetStringConst( tree_top, "id" ); osrfLogDebug( OSRF_LOG_MARK, "Top of the org tree is %s", root_org_unit_id ); - jsonObjectFree( result ); - strcpy( cached_root_id, root_org_unit_id ); - return root_org_unit_id; + jsonObjectFree( result ); + return cached_root_id; } /** diff --git a/Open-ILS/src/c-apps/oils_utils.c b/Open-ILS/src/c-apps/oils_utils.c index d8b69292d3..3739413784 100644 --- a/Open-ILS/src/c-apps/oils_utils.c +++ b/Open-ILS/src/c-apps/oils_utils.c @@ -22,24 +22,67 @@ osrfHash* oilsInitIDL(const char* idl_filename) { if (!oilsIDLInit( filename )) { osrfLogError(OSRF_LOG_MARK, "Problem loading IDL file [%s]!", filename); - if(freeable_filename) free(freeable_filename); + if(freeable_filename) + free(freeable_filename); return NULL; } - if(freeable_filename) free(freeable_filename); + if(freeable_filename) + free(freeable_filename); return oilsIDL(); } +/** + @brief Return a const string with the value of a specified column in a row object. + @param object Pointer to the row object. + @param field Name of the column. + @return Pointer to a const string representing the value of the specified column, + or NULL in case of error. + + The row object must be a JSON_ARRAY with a classname. The column value must be a + JSON_STRING or a JSON_NUMBER. Any other object type results in a return of NULL. + + The return value points into the internal contents of the row object, which + retains ownership. +*/ +const char* oilsFMGetStringConst( const jsonObject* object, const char* field ) { + return jsonObjectGetString(oilsFMGetObject( object, field )); +} + +/** + @brief Return a string with the value of a specified column in a row object. + @param object Pointer to the row object. + @param field Name of the column. + @return Pointer to a newly allocated string representing the value of the specified column, + or NULL in case of error. + + The row object must be a JSON_ARRAY with a classname. The column value must be a + JSON_STRING or a JSON_NUMBER. Any other object type results in a return of NULL. + + The calling code is responsible for freeing the returned string by calling free(). + */ char* oilsFMGetString( const jsonObject* object, const char* field ) { return jsonObjectToSimpleString(oilsFMGetObject( object, field )); } +/** + @brief Return a pointer to the value of a specified column in a row object. + @param object Pointer to the row object. + @param field Name of the column. + @return Pointer to the jsonObject representing the value of the specified column, or NULL + in case of error. + The row object must be a JSON_ARRAY with a classname. + + The return value may point to a JSON_NULL, JSON_STRING, JSON_NUMBER, or JSON_ARRAY. It + points into the internal contents of the row object, which retains ownership. +*/ const jsonObject* oilsFMGetObject( const jsonObject* object, const char* field ) { if(!(object && field)) return NULL; if( object->type != JSON_ARRAY || !object->classname ) return NULL; int pos = fm_ntop(object->classname, field); - if( pos > -1 ) return jsonObjectGetIndex( object, pos ); + if( pos > -1 ) + return jsonObjectGetIndex( object, pos ); return NULL; } @@ -68,7 +111,10 @@ long oilsFMGetObjectId( const jsonObject* obj ) { long id = -1; if(!obj) return id; char* ids = oilsFMGetString( obj, "id" ); - if(ids) { id = atol(ids); free(ids); } + if(ids) { + id = atol(ids); + free(ids); + } return id; } @@ -78,6 +124,8 @@ oilsEvent* oilsUtilsCheckPerms( int userid, int orgid, char* permissions[], int int i; oilsEvent* evt = NULL; + // Find the root org unit, i.e. the one with no parent. + // Assumption: there is only one org unit with no parent. if (orgid == -1) { jsonObject* where_clause = jsonParse( "{\"parent_ou\":null}" ); jsonObject* org = oilsUtilsQuickReq( @@ -87,60 +135,116 @@ oilsEvent* oilsUtilsCheckPerms( int userid, int orgid, char* permissions[], int ); jsonObjectFree( where_clause ); - orgid = (int)jsonObjectGetNumber( oilsFMGetObject( org, "id" ) ); + orgid = (int)jsonObjectGetNumber( oilsFMGetObject( org, "id" ) ); - jsonObjectFree(org); - } + jsonObjectFree(org); + } for( i = 0; i < size && permissions[i]; i++ ) { char* perm = permissions[i]; jsonObject* params = jsonParseFmt("[%d, \"%s\", %d]", userid, perm, orgid); - jsonObject* o = oilsUtilsQuickReq( "open-ils.storage", + jsonObject* o = oilsUtilsQuickReq( "open-ils.storage", "open-ils.storage.permission.user_has_perm", params ); char* r = jsonObjectToSimpleString(o); - if(r && !strcmp(r, "0")) + if(r && !strcmp(r, "0")) evt = oilsNewEvent3( OSRF_LOG_MARK, OILS_EVENT_PERM_FAILURE, perm, orgid ); jsonObjectFree(params); jsonObjectFree(o); free(r); - if(evt) break; + if(evt) + break; } return evt; } +/** + @brief Perform a remote procedure call. + @param service The name of the service to invoke. + @param method The name of the method to call. + @param params The parameters to be passed to the method, if any. + @return A copy of whatever the method returns as a result, or a JSON_NULL if the method + doesn't return anything. + + If the @a params parameter points to a JSON_ARRAY, pass each element of the array + as a separate parameter. If it points to any other kind of jsonObject, pass it as a + single parameter. If it is NULL, pass no parameters. + + The calling code is responsible for freeing the returned object by calling jsonObjectFree(). +*/ jsonObject* oilsUtilsQuickReq( const char* service, const char* method, const jsonObject* params ) { if(!(service && method)) return NULL; + osrfLogDebug(OSRF_LOG_MARK, "oilsUtilsQuickReq(): %s - %s", service, method ); - osrfAppSession* session = osrfAppSessionClientInit( service ); + + // Open an application session with the service, and send the request + osrfAppSession* session = osrfAppSessionClientInit( service ); int reqid = osrfAppSessionSendRequest( session, params, method, 1 ); - osrfMessage* omsg = osrfAppSessionRequestRecv( session, reqid, 60 ); - jsonObject* result = jsonObjectClone(osrfMessageGetResult(omsg)); + + // Get the response + osrfMessage* omsg = osrfAppSessionRequestRecv( session, reqid, 60 ); + jsonObject* result = jsonObjectClone( osrfMessageGetResult(omsg) ); + + // Clean up osrfMessageFree(omsg); osrfAppSessionFree(session); return result; } +/** + @brief Call a method of the open-ils.storage service. + @param method Name of the method. + @param params Parameters to be passed to the method, if any. + @return A copy of whatever the method returns as a result, or a JSON_NULL if the method + doesn't return anything. + + If the @a params parameter points to a JSON_ARRAY, pass each element of the array + as a separate parameter. If it points to any other kind of jsonObject, pass it as a + single parameter. If it is NULL, pass no parameters. + + The calling code is responsible for freeing the returned object by calling jsonObjectFree(). +*/ jsonObject* oilsUtilsStorageReq( const char* method, const jsonObject* params ) { return oilsUtilsQuickReq( "open-ils.storage", method, params ); } +/** + @brief Call a method of the open-ils.cstore service. + @param method Name of the method. + @param params Parameters to be passed to the method, if any. + @return A copy of whatever the method returns as a result, or a JSON_NULL if the method + doesn't return anything. + + If the @a params parameter points to a JSON_ARRAY, pass each element of the array + as a separate parameter. If it points to any other kind of jsonObject, pass it as a + single parameter. If it is NULL, pass no parameters. + + The calling code is responsible for freeing the returned object by calling jsonObjectFree(). +*/ jsonObject* oilsUtilsCStoreReq( const char* method, const jsonObject* params ) { return oilsUtilsQuickReq("open-ils.cstore", method, params); } +/** + @brief Given a username, fetch the corresponding row from the actor.usr table, if any. + @param name The username for which to search. + @return A Fieldmapper object for the relevant row in the actor.usr table, if it exists; + or a JSON_NULL if it doesn't. + + The calling code is responsible for freeing the returned object by calling jsonObjectFree(). +*/ jsonObject* oilsUtilsFetchUserByUsername( const char* name ) { if(!name) return NULL; jsonObject* params = jsonParseFmt("{\"usrname\":\"%s\"}", name); - jsonObject* user = oilsUtilsQuickReq( + jsonObject* user = oilsUtilsQuickReq( "open-ils.cstore", "open-ils.cstore.direct.actor.user.search", params ); jsonObjectFree(params); @@ -149,6 +253,17 @@ jsonObject* oilsUtilsFetchUserByUsername( const char* name ) { return user; } +/** + @brief Given a barcode, fetch the corresponding row from the actor.usr table, if any. + @param name The barcode for which to search. + @return A Fieldmapper object for the relevant row in the actor.usr table, if it exists; + or a JSON_NULL if it doesn't. + + Look up the barcode in actor.card. Follow a foreign key from there to get a row in + actor.usr. + + The calling code is responsible for freeing the returned object by calling jsonObjectFree(). +*/ jsonObject* oilsUtilsFetchUserByBarcode(const char* barcode) { if(!barcode) return NULL; @@ -159,14 +274,18 @@ jsonObject* oilsUtilsFetchUserByBarcode(const char* barcode) { "open-ils.cstore", "open-ils.cstore.direct.actor.card.search", params ); jsonObjectFree(params); - if(!card) return NULL; + if(!card) + return NULL; // No such card + // Get the user's id as a double char* usr = oilsFMGetString(card, "usr"); jsonObjectFree(card); - if(!usr) return NULL; + if(!usr) + return NULL; // No user id (shouldn't happen) double iusr = strtod(usr, NULL); free(usr); + // Look up the user in actor.usr params = jsonParseFmt("[%f]", iusr); jsonObject* user = oilsUtilsQuickReq( "open-ils.cstore", "open-ils.cstore.direct.actor.user.retrieve", params); @@ -182,9 +301,9 @@ char* oilsUtilsFetchOrgSetting( int orgid, const char* setting ) { jsonObject* set = oilsUtilsQuickReq( "open-ils.actor", - "open-ils.actor.ou_setting.ancestor_default", params); + "open-ils.actor.ou_setting.ancestor_default", params); - char* value = jsonObjectToSimpleString(jsonObjectGetKey(set, "value")); + char* value = jsonObjectToSimpleString(jsonObjectGetKey(set, "value")); jsonObjectFree(params); jsonObjectFree(set); osrfLogDebug(OSRF_LOG_MARK, "Fetched org [%d] setting: %s => %s", orgid, setting, value); @@ -201,7 +320,7 @@ char* oilsUtilsLogin( const char* uname, const char* passwd, const char* type, i jsonObject* params = jsonParseFmt("[\"%s\"]", uname); - jsonObject* o = oilsUtilsQuickReq( + jsonObject* o = oilsUtilsQuickReq( "open-ils.auth", "open-ils.auth.authenticate.init", params ); const char* seed = jsonObjectGetString(o); @@ -221,7 +340,8 @@ char* oilsUtilsLogin( const char* uname, const char* passwd, const char* type, i if(o) { const char* tok = jsonObjectGetString( jsonObjectGetKey(jsonObjectGetKey(o,"payload"), "authtoken")); - if(tok) token = strdup(tok); + if( tok ) + token = strdup( tok ); } free(fullhash); @@ -235,7 +355,7 @@ char* oilsUtilsLogin( const char* uname, const char* passwd, const char* type, i jsonObject* oilsUtilsFetchWorkstation( long id ) { jsonObject* p = jsonParseFmt("[%ld]", id); jsonObject* r = oilsUtilsQuickReq( - "open-ils.storage", + "open-ils.storage", "open-ils.storage.direct.actor.workstation.retrieve", p ); jsonObjectFree(p); return r; @@ -243,11 +363,8 @@ jsonObject* oilsUtilsFetchWorkstation( long id ) { jsonObject* oilsUtilsFetchWorkstationByName( const char* name ) { jsonObject* p = jsonParseFmt("{\"name\":\"%s\"}", name); - jsonObject* r = oilsUtilsCStoreReq( - "open-ils.cstore.direct.actor.workstation.search", p); + jsonObject* r = oilsUtilsCStoreReq( + "open-ils.cstore.direct.actor.workstation.search", p); jsonObjectFree(p); - return r; + return r; } - - - -- 2.43.2