From: miker Date: Mon, 26 Nov 2007 18:25:09 +0000 (+0000) Subject: Big cleanup of the C auth app from Scott McKellar: X-Git-Url: https://old-git.evergreen-ils.org/?a=commitdiff_plain;h=1ae174ddb78bd56bcbc9e17a9fe7d04febce5146;p=Evergreen.git Big cleanup of the C auth app from Scott McKellar: 1. Apply the const qualifier where possible. 2. Plug memory leaks. 3. Make some global things static. 4. Some minor tidying-up. Details: 1. The variables __oilsAuthOPACTimeout, __oilsAuthStaffTimeout and and __oilsAuthOverrideTimeout are now static, since no other file references them. Also I reduced the leading underscores to one each. 2. In several places I replaced jsonObjectGetKey() with jsonObjectGetKeyConst(). 3. In oilsAuthInit(): I plugged a memory leak by moving the freeing of username out of an else block and into the code following. 4. In oilsAuthVerifyPassword(): I plugged a memory leak in the case of an early return. 5. In oilsAuthVerifyWorkstation: I plugged a memory leak by freeing workstation at the end. 6. In oilsAuthCheckCard(): I eliminated the ctx and userObj parameters. The function didn't do anything with them except to verify that they weren't NULL. I transferred that responsibility to the calling code in oilsAuthComplete() (where I also plugged another memory leak). 7. Also in oilsAuthCheckCard(): I plugged a memory leak by freeing params and card. Also I rearranged the last few lines a bit so that we free active in only one place. 8. In oilsAuthComplete(): I plugged a number of memory leaks caused by a failure to free userObj in various early returns. 9. Also in oilsAuthComplete(): In order to be able make uname a const pointer, but still be able to delete it when necessary, I created an intermdiate non-const pointer named freeable_uname -- which also takes the place of the original freeuname variable used as a boolean. 10. In oilsAuthSessionRetrieve(): I moved the freeing of evt out of the if-else structure and put it at the end, to make sure that none of the branches would miss it. 11. Also in oilsAuthSessionRetrieve(): one of the branches of the if-else had a second variable named evt, hiding the variable of the same name declared in an enclosing scope. That's not a bug, but it's confusing. I renamed it to evt2. 12. The following functions are now static: oilsAuthCheckLoginPerm() oilsAuthVerifyPassword() oilsAuthGetTimeout() oilsAuthHandleLoginOK() oilsAuthVerifyWorkstation() _oilsAuthResetTimeout() 13. I added the const qualifier to various parameters and local variables. git-svn-id: svn://svn.open-ils.org/ILS/trunk@8115 dcc99617-32d9-48b4-a31d-7c20da2025e4 --- diff --git a/Open-ILS/src/c-apps/oils_auth.c b/Open-ILS/src/c-apps/oils_auth.c index 7cc9c0ac9a..07e71ad169 100644 --- a/Open-ILS/src/c-apps/oils_auth.c +++ b/Open-ILS/src/c-apps/oils_auth.c @@ -18,9 +18,9 @@ int osrfAppInitialize(); int osrfAppChildInit(); -int __oilsAuthOPACTimeout = 0; -int __oilsAuthStaffTimeout = 0; -int __oilsAuthOverrideTimeout = 0; +static int _oilsAuthOPACTimeout = 0; +static int _oilsAuthStaffTimeout = 0; +static int _oilsAuthOverrideTimeout = 0; int osrfAppInitialize() { @@ -115,10 +115,10 @@ int oilsAuthInit( osrfMethodContext* ctx ) { free(seed); free(md5seed); free(key); - free(username); } jsonObjectFree(resp); + free(username); return 0; } @@ -131,8 +131,8 @@ int oilsAuthInit( osrfMethodContext* ctx ) { * @return -1 if the permission check failed, 0 if ther permission * is granted */ -int oilsAuthCheckLoginPerm( - osrfMethodContext* ctx, jsonObject* userObj, char* type ) { +static int oilsAuthCheckLoginPerm( + osrfMethodContext* ctx, const jsonObject* userObj, const char* type ) { if(!(userObj && type)) return -1; oilsEvent* perm = NULL; @@ -164,8 +164,8 @@ int oilsAuthCheckLoginPerm( * Returns 0 otherwise * Returns -1 on error */ -int oilsAuthVerifyPassword( - osrfMethodContext* ctx, jsonObject* userObj, char* uname, char* password ) { +static int oilsAuthVerifyPassword( const osrfMethodContext* ctx, + const jsonObject* userObj, const char* uname, const char* password ) { int ret = 0; char* realPassword = oilsFMGetString( userObj, "passwd" ); /**/ @@ -180,7 +180,11 @@ int oilsAuthVerifyPassword( osrfLogInternal(OSRF_LOG_MARK, "oilsAuth retrieved real password: [%s]", realPassword); osrfLogDebug(OSRF_LOG_MARK, "oilsAuth retrieved seed from cache: %s", seed ); char* maskedPw = md5sum( "%s%s", seed, realPassword ); - if(!maskedPw) return -1; + if(!maskedPw) { + free(realPassword); + free(seed); + return -1; + } osrfLogDebug(OSRF_LOG_MARK, "oilsAuth generated masked password %s. " "Testing against provided password %s", maskedPw, password ); @@ -201,28 +205,28 @@ int oilsAuthVerifyPassword( * setting for the home org unit of the user logging in * 3. If that setting is not defined, we use the configured defaults */ -double oilsAuthGetTimeout( jsonObject* userObj, char* type, double orgloc ) { +static double oilsAuthGetTimeout( const jsonObject* userObj, const char* type, double orgloc ) { - if(!__oilsAuthOPACTimeout) { /* Load the default timeouts */ + if(!_oilsAuthOPACTimeout) { /* Load the default timeouts */ - __oilsAuthOPACTimeout = + _oilsAuthOPACTimeout = jsonObjectGetNumber( osrf_settings_host_value_object( "/apps/open-ils.auth/app_settings/default_timeout/opac")); - __oilsAuthStaffTimeout = + _oilsAuthStaffTimeout = jsonObjectGetNumber( osrf_settings_host_value_object( "/apps/open-ils.auth/app_settings/default_timeout/staff" )); - __oilsAuthOverrideTimeout = + _oilsAuthOverrideTimeout = jsonObjectGetNumber( osrf_settings_host_value_object( "/apps/open-ils.auth/app_settings/default_timeout/temp" )); osrfLogInfo(OSRF_LOG_MARK, "Set default auth timetouts: opac => %d : staff => %d : temp => %d", - __oilsAuthOPACTimeout, __oilsAuthStaffTimeout, __oilsAuthOverrideTimeout ); + _oilsAuthOPACTimeout, _oilsAuthStaffTimeout, _oilsAuthOverrideTimeout ); } char* setting = NULL; @@ -246,9 +250,9 @@ double oilsAuthGetTimeout( jsonObject* userObj, char* type, double orgloc ) { timeout = oilsUtilsFetchOrgSetting( (int) home_ou, setting ); } if(!timeout) { - if(!strcmp(type, OILS_AUTH_STAFF)) return __oilsAuthStaffTimeout; - if(!strcmp(type, OILS_AUTH_TEMP)) return __oilsAuthOverrideTimeout; - return __oilsAuthOPACTimeout; + if(!strcmp(type, OILS_AUTH_STAFF)) return _oilsAuthStaffTimeout; + if(!strcmp(type, OILS_AUTH_TEMP)) return _oilsAuthOverrideTimeout; + return _oilsAuthOPACTimeout; } } @@ -263,8 +267,8 @@ double oilsAuthGetTimeout( jsonObject* userObj, char* type, double orgloc ) { * Returns the event that should be returned to the user. * Event must be freed */ -oilsEvent* oilsAuthHandleLoginOK( - jsonObject* userObj, char* uname, char* type, double orgloc, char* workstation ) { +static oilsEvent* oilsAuthHandleLoginOK( jsonObject* userObj, const char* uname, + const char* type, double orgloc, const char* workstation ) { oilsEvent* response; @@ -288,7 +292,7 @@ oilsEvent* oilsAuthHandleLoginOK( char* authKey = va_list_to_string( "%s%s", OILS_AUTH_CACHE_PRFX, authToken ); - char* ws = (workstation) ? workstation : ""; + const char* ws = (workstation) ? workstation : ""; osrfLogActivity(OSRF_LOG_MARK, "successful login: username=%s, authtoken=%s, workstation=%s", uname, authToken, ws ); @@ -309,8 +313,8 @@ oilsEvent* oilsAuthHandleLoginOK( return response; } -oilsEvent* oilsAuthVerifyWorkstation( - osrfMethodContext* ctx, jsonObject* userObj, char* ws ) { +static oilsEvent* oilsAuthVerifyWorkstation( + const osrfMethodContext* ctx, jsonObject* userObj, const char* ws ) { osrfLogInfo(OSRF_LOG_MARK, "Attaching workstation to user at login: %s", ws); jsonObject* workstation = oilsUtilsFetchWorkstationByName(ws); if(!workstation) return oilsNewEvent(OSRF_LOG_MARK, "WORKSTATION_NOT_FOUND"); @@ -320,29 +324,33 @@ oilsEvent* oilsAuthVerifyWorkstation( oilsFMSetString(userObj, "wsid", LONGSTR); oilsFMSetString(userObj, "ws_ou", orgid); free(orgid); + jsonObjectFree(workstation); return NULL; } /* see if the card used to login is marked as barred */ -static oilsEvent* oilsAuthCheckCard( osrfMethodContext* ctx, jsonObject* userObj, char* barcode) { - if(!(ctx && userObj && barcode)) return NULL; +static oilsEvent* oilsAuthCheckCard( const char* barcode ) { + if(!barcode) return NULL; osrfLogDebug(OSRF_LOG_MARK, "Checking to see if barcode %s is active", barcode); jsonObject* params = jsonParseStringFmt("{\"barcode\":\"%s\"}", barcode); jsonObject* card = oilsUtilsQuickReq( "open-ils.cstore", "open-ils.cstore.direct.actor.card.search", params ); + jsonObjectFree(params); char* active = oilsFMGetString(card, "active"); + jsonObjectFree(card); + + oilsEvent* return_event = NULL; if( ! oilsUtilsIsDBTrue(active) ) { - free(active); osrfLogInfo(OSRF_LOG_MARK, "barcode %s is not active, returning event", barcode); - return oilsNewEvent(OSRF_LOG_MARK, "PATRON_CARD_INACTIVE"); + return_event = oilsNewEvent(OSRF_LOG_MARK, "PATRON_CARD_INACTIVE"); } free(active); - return NULL; + return return_event; } @@ -350,16 +358,16 @@ static oilsEvent* oilsAuthCheckCard( osrfMethodContext* ctx, jsonObject* userObj int oilsAuthComplete( osrfMethodContext* ctx ) { OSRF_METHOD_VERIFY_CONTEXT(ctx); - jsonObject* args = jsonObjectGetIndex(ctx->params, 0); + const jsonObject* args = jsonObjectGetIndex(ctx->params, 0); - char* uname = jsonObjectGetString(jsonObjectGetKey(args, "username")); - char* password = jsonObjectGetString(jsonObjectGetKey(args, "password")); - char* type = jsonObjectGetString(jsonObjectGetKey(args, "type")); - double orgloc = jsonObjectGetNumber(jsonObjectGetKey(args, "org")); - char* workstation = jsonObjectGetString(jsonObjectGetKey(args, "workstation")); - char* barcode = jsonObjectToSimpleString(jsonObjectGetKey(args, "barcode")); + const char* uname = jsonObjectGetString(jsonObjectGetKeyConst(args, "username")); + const char* password = jsonObjectGetString(jsonObjectGetKeyConst(args, "password")); + const char* type = jsonObjectGetString(jsonObjectGetKeyConst(args, "type")); + double orgloc = jsonObjectGetNumber(jsonObjectGetKeyConst(args, "org")); + const char* workstation = jsonObjectGetString(jsonObjectGetKeyConst(args, "workstation")); + char* barcode = jsonObjectToSimpleString(jsonObjectGetKeyConst(args, "barcode")); - char* ws = (workstation) ? workstation : ""; + const char* ws = (workstation) ? workstation : ""; if(!type) type = OILS_AUTH_STAFF; @@ -392,6 +400,7 @@ int oilsAuthComplete( osrfMethodContext* ctx ) { passOK = oilsAuthVerifyPassword( ctx, userObj, barcode, password ); if( passOK < 0 ) { + jsonObjectFree(userObj); free(barcode); return passOK; } @@ -402,6 +411,7 @@ int oilsAuthComplete( osrfMethodContext* ctx ) { response = oilsNewEvent(OSRF_LOG_MARK, "PATRON_INACTIVE"); osrfAppRespondComplete( ctx, oilsEventToJSON(response) ); oilsEventFree(response); + jsonObjectFree(userObj); free(barcode); free(active); return 0; @@ -409,9 +419,10 @@ int oilsAuthComplete( osrfMethodContext* ctx ) { free(active); /* then see if the barcode they used is active */ - if( barcode && (response = oilsAuthCheckCard( ctx, userObj, barcode )) ) { + if( barcode && ctx && userObj && (response = oilsAuthCheckCard( barcode )) ) { osrfAppRespondComplete( ctx, oilsEventToJSON(response) ); oilsEventFree(response); + jsonObjectFree(userObj); free(barcode); return 0; } @@ -444,10 +455,9 @@ int oilsAuthComplete( osrfMethodContext* ctx ) { free(orgid); } - int freeuname = 0; + char* freeable_uname = NULL; if(!uname) { - uname = oilsFMGetString( userObj, "usrname" ); - freeuname = 1; + uname = freeable_uname = oilsFMGetString( userObj, "usrname" ); } if( passOK ) { @@ -463,7 +473,7 @@ int oilsAuthComplete( osrfMethodContext* ctx ) { oilsEventFree(response); free(barcode); - if(freeuname) free(uname); + if(freeable_uname) free(freeable_uname); return 0; } @@ -473,7 +483,7 @@ int oilsAuthComplete( osrfMethodContext* ctx ) { int oilsAuthSessionDelete( osrfMethodContext* ctx ) { OSRF_METHOD_VERIFY_CONTEXT(ctx); - char* authToken = jsonObjectGetString( jsonObjectGetIndex(ctx->params, 0) ); + const char* authToken = jsonObjectGetString( jsonObjectGetIndex(ctx->params, 0) ); jsonObject* resp = NULL; if( authToken ) { @@ -492,7 +502,7 @@ int oilsAuthSessionDelete( osrfMethodContext* ctx ) { /** Resets the auth login timeout * @return The event object, OILS_EVENT_SUCCESS, or OILS_EVENT_NO_SESSION */ -oilsEvent* _oilsAuthResetTimeout( char* authToken ) { +static oilsEvent* _oilsAuthResetTimeout( const char* authToken ) { if(!authToken) return NULL; oilsEvent* evt = NULL; @@ -508,7 +518,7 @@ oilsEvent* _oilsAuthResetTimeout( char* authToken ) { } else { - timeout = jsonObjectGetNumber( jsonObjectGetKey( cacheObj, "authtime")); + timeout = jsonObjectGetNumber( jsonObjectGetKeyConst( cacheObj, "authtime")); osrfCacheSetExpire( timeout, key ); jsonObject* payload = jsonNewNumberObject(timeout); evt = oilsNewEvent2(OSRF_LOG_MARK, OILS_EVENT_SUCCESS, payload); @@ -522,7 +532,7 @@ oilsEvent* _oilsAuthResetTimeout( char* authToken ) { int oilsAuthResetTimeout( osrfMethodContext* ctx ) { OSRF_METHOD_VERIFY_CONTEXT(ctx); - char* authToken = jsonObjectGetString( jsonObjectGetIndex(ctx->params, 0)); + const char* authToken = jsonObjectGetString( jsonObjectGetIndex(ctx->params, 0)); oilsEvent* evt = _oilsAuthResetTimeout(authToken); osrfAppRespondComplete( ctx, oilsEventToJSON(evt) ); oilsEventFree(evt); @@ -533,7 +543,7 @@ int oilsAuthResetTimeout( osrfMethodContext* ctx ) { int oilsAuthSessionRetrieve( osrfMethodContext* ctx ) { OSRF_METHOD_VERIFY_CONTEXT(ctx); - char* authToken = jsonObjectGetString( jsonObjectGetIndex(ctx->params, 0)); + const char* authToken = jsonObjectGetString( jsonObjectGetIndex(ctx->params, 0)); jsonObject* cacheObj = NULL; oilsEvent* evt = NULL; @@ -543,7 +553,6 @@ int oilsAuthSessionRetrieve( osrfMethodContext* ctx ) { if( evt && strcmp(evt->event, OILS_EVENT_SUCCESS) ) { osrfAppRespondComplete( ctx, oilsEventToJSON(evt) ); - oilsEventFree(evt); } else { @@ -551,12 +560,12 @@ int oilsAuthSessionRetrieve( osrfMethodContext* ctx ) { char* key = va_list_to_string("%s%s", OILS_AUTH_CACHE_PRFX, authToken ); cacheObj = osrfCacheGetObject( key ); if(cacheObj) { - osrfAppRespondComplete( ctx, jsonObjectGetKey( cacheObj, "userobj")); + osrfAppRespondComplete( ctx, jsonObjectGetKeyConst( cacheObj, "userobj")); jsonObjectFree(cacheObj); } else { - oilsEvent* evt = oilsNewEvent(OSRF_LOG_MARK, OILS_EVENT_NO_SESSION); - osrfAppRespondComplete( ctx, oilsEventToJSON(evt) ); /* should be event.. */ - oilsEventFree(evt); + oilsEvent* evt2 = oilsNewEvent(OSRF_LOG_MARK, OILS_EVENT_NO_SESSION); + osrfAppRespondComplete( ctx, oilsEventToJSON(evt2) ); /* should be event.. */ + oilsEventFree(evt2); } free(key); } @@ -565,9 +574,11 @@ int oilsAuthSessionRetrieve( osrfMethodContext* ctx ) { evt = oilsNewEvent(OSRF_LOG_MARK, OILS_EVENT_NO_SESSION); osrfAppRespondComplete( ctx, oilsEventToJSON(evt) ); - oilsEventFree(evt); } + if(evt) + oilsEventFree(evt); + return 0; }