CodeSOD: Capitalizing on Memories

Gavin inherited some "very old" C code, originally developed for old Windows systems. The code is loaded with a number of reinvented wheels.

For example, they needed to do a case insensitive string comparison. Now, instead of using stricmp, the case insensitive string comparison, they wrote this:

int uppercmp( char *sStr1, char *sStr2 ){char *sUStr1;char *sUStr2;int iRet;if ( ( sStr1 == NULL ) || ( sStr2 == NULL ) ) return -1;__try{sUStr1 = strupr(strdup( sStr1 ));sUStr2 = strupr(strdup( sStr2 ));iRet = strcmp( sUStr1, sUStr2 );return iRet;}__finally{FreeIfNotNULL(sUStr1);FreeIfNotNULL(sUStr2);}return}

This uses Microsoft's structured exception handling extensions to C, hence the __try. SEH is its own weird beast, with huge amounts of caveats and gotchas and weirdness. It's honestly more interesting than this function, which isn't itself a WTF- it's just bad. They duplicate the input strings, convert them to upper case, and then compare. The real ugly thing is the return value- a -1 if either input string is NULL, but also a -1 from strcmp if sUStr1 < sUstr2. And of course, an empty return if anything causes an exception.

This code was so nice, they wrote it twice, as here's their StringEqual function.

BOOL StringEqual( IN char *sSearch, IN DWORD dwSearchLen, IN char *sToken, IN DWORD dwTokenLen, IN BOOL bCaseInsensitive ){char *sUpperSearch;char *sUpperToken;BOOLbStringEqual = FALSE;// If the lengths are mismatched or the parameters NULL, its not a matchif ( (dwSearchLen < dwTokenLen) || (sToken == NULL) || (sSearch == NULL) ){return FALSE;}// Check for case insensitivityif ( bCaseInsensitive ){__try{// Make upper case versions of the comparitor and the comparitand (my own words ;)sUpperSearch = strdup( sSearch );sUpperToken = strdup( sToken );_strupr( sUpperSearch );_strupr( sUpperToken );// And return a TRUE if there is a matchreturn ( strncmp( sUpperSearch, sUpperToken, dwTokenLen ) == 0 );}__finally{FreeIfNotNULL(sUpperSearch);FreeIfNotNULL(sUpperToken);}}else{// Return if there is a case sensitive matchreturn ( strncmp( sSearch, sToken, dwTokenLen ) == 0 );}// This line never gets called, but lets keep it here to make the// warning go awayreturn FALSE;}

Here, they're wrapping strncmp, which expects the caller to supply the length of the strings (instead of relying on null terminators). That's arguably a good practice, but it's foiled by using strdup, and _strupr- which are looking for null terminators.

With all these homebrew string handling functions, would you be shocked to know they have a homebrew memory management function too? Here's OverlappedMemcpy.

void OverlappedMemcpy( IN OUT void *vDest, IN void *vSrc, IN DWORD dwLen ){void *vTemp;__try{// Store the from storevTemp = malloc( dwLen );// Copy the sourcememcpy( vTemp, vSrc, dwLen );// .. to the dest through the temporary intermidiatememcpy( vDest, vTemp, dwLen );}__finally{FreeIfNotNULL(vTemp);}}

I do not know what about this is "overlapped". It's just a regular memcpy where we added an intermediate step of copying into a temporary buffer before copying to the destination buffer. So it's just really inefficient, for absolutely no benefit to anyone.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!