It is currently Sun Apr 23, 2017 8:47 am

All times are UTC - 5 hours




 Page 1 of 1 [ 12 posts ] 
Author Message
 Post subject: On using safer functions inplace of depricated ones!!!
PostPosted: Thu Oct 21, 2010 10:26 pm 

Joined: Sat Aug 16, 2008 7:58 am
Posts: 447
Ok here is some code that is with in the game engine...
bool Node::SetName( char *szName )
{
   // Check If Name Is Valid
   if ( szName == NULL )
   {
      return false;
   }

   // If A Name Already Exists Delete It First
   SAFE_DELETE_ARRAY( m_pszName );

   // Allocate Memory For The Name
   m_pszName = new char[strlen(szName)+1];
   if ( m_pszName == NULL )
   {
      return false;
   }

   strcpy( m_pszName, szName );
   return true;

}


the compiler for vs 2008 gives the warning about strcpy(...) about being unsafe and will be depricate and suggests that I should use strcpy_s() instead

ok np, strcpy_s() takes 3 params... the first being *dest, second being size in bytes, and third being source...

so if we look at the original code here a char* named *szName is passed into function.. we check to see if valid... then if anything is in the variable we delete it... then we assign this to our member variable using the new char[strlen(szName)+1] operator and if the member variable is null we return false from the function... otherwise we copy the name that is passed into this function...

I try to do this strcpy_s( m_pszName, sizeof( m_pszName ), szName );
but it doesn't work... i also tried the second param as sizeof( szName )
but get the same error.

It compiles fine but when I run the code I get this....

Debug Assertion Failed!
...
...
Expression(L"Buffer is too small" && 0)

How would I go about implementing this using the strcpy_s funciton instead of strcpy ... I checked msdn and that didn't help me much either, since in our case here, we are allocating memory dynamically.

There are other functions that I may have trouble with trying to convert them to their equivalent safer versions, but I will post them in a new thread when needed.


Offline
 Profile  
 
 Post subject:
PostPosted: Fri Oct 22, 2010 9:12 am 

Joined: Sun Dec 02, 2007 3:24 am
Posts: 13
Location: Galati
Hi there. You can use strcpy_s like you were using strcpy, i.e. , you don't need to specify the size of the destination string buffer.

Look at the Example section on the strcpy_s MSDN page: http://msdn.microsoft.com/en-us/library/td1esda9(VS.80).aspx


Offline
 Profile  
 
 Post subject:
PostPosted: Fri Oct 22, 2010 7:24 pm 

Joined: Sat Aug 16, 2008 7:58 am
Posts: 447
Yeah, I tried using it without the size, but then the compiler complains that the function does not take 2 arguments


Offline
 Profile  
 
 Post subject:
PostPosted: Sat Oct 23, 2010 2:39 am 

Joined: Sun Dec 02, 2007 3:24 am
Posts: 13
Location: Galati
Hm... have you tried using the _countof() macro instead of using sizeof ?
Like this ?
strcpy_s( m_pszName, _countof( m_pszName ), szName );


Offline
 Profile  
 
 Post subject:
PostPosted: Sat Oct 23, 2010 11:08 am 

Joined: Sat Aug 16, 2008 7:58 am
Posts: 447
when I try to use the _countof macro, I get this message, when I try to compile the code....

>c:\users\skilz80\documents\visual studio 2008\projects\virtualworld\scenelib\node.cpp(61) : error C2784: 'char (*__countof_helper(_CountofType (&)[_SizeOfArray]))[_SizeOfArray]' : could not deduce template argument for '_CountofType (&)[_SizeOfArray]' from 'char *'


Offline
 Profile  
 
 Post subject:
PostPosted: Sat Oct 23, 2010 11:17 am 

Joined: Sat Aug 16, 2008 7:58 am
Posts: 447
ok, I tried a few other techniques, and this is what I have came up with...

    // Allocate Memory For The Name
    m_pszName = new char[strlen(szName)+1];
    if ( m_pszName == NULL )
    {
        return false;
    }

    int iStrSize = NULL;
    iStrSize = sizeof(m_pszName)+3;

    strcpy_s( m_pszName, iStrSize, szName );


Here I created an integer variable to hold the size of the string...
if I add 1 or 2 to the string I get the same error as before
with the buffer being to small...

If I add 4 or more to the size I get an error message when I close the program with possible corruption in the heap...

but, if I run it like this with +3 it works fine....

Any thoughts on why this is ?


Offline
 Profile  
 
 Post subject:
PostPosted: Sun Oct 24, 2010 4:54 am 

Joined: Sat Aug 16, 2008 7:58 am
Posts: 447
Ok, in the post above the +3 worked with the lvl parser, but after I
finished implementing the caligari parser, I was getting the same bug with buffer too small


Offline
 Profile  
 
 Post subject:
PostPosted: Sun Oct 24, 2010 6:30 am 
Site Admin

Joined: Sun Feb 11, 2007 8:59 am
Posts: 1094
Location: Ontario Canada
The size that you specify as the second parameter should represent the maximum size of your destination buffer. Therefore, use:

   unsigned int uiSize = strlen( szName ) + 1;
    m_pszName = new char[uiSize];
    if ( m_pszName == NULL )
    {
        return false;
    }

    strcpy_s( m_pszName, uiSize, szName );


The call to sizeof(m_pszName) will return 4 because the size of a pointer is 4 bytes on a 32bit machine. This does not give you the size of the memory allocated using new.


Offline
 Profile  
 
 Post subject: How to ruin your life - a self-help tool :)
PostPosted: Sun Oct 24, 2010 4:10 pm 

Joined: Wed Aug 06, 2008 7:53 pm
Posts: 182
Location: Russia
One question about the semantics of the method bool Node::SetName( char* szName ). Should it be considered as an error the attempt to assign NULL-pointer as a node name or it is just an expression of intention to reset the node-name? My code allows such assignment:
inline void Node::SetName( const TCHAR* name )
{
    if ( m_name ) StrFree( m_name );
    m_name = name ? StrDup( name ) : NULL;
}

In other words, one can reset the name of the node to the NULL instead of returning meaningless ‘false’. Most likely we won’t check the return value anyway.
To forget about internals of allocation/deallocation of memory for fixed-size strings once and for all I added to the Utilities unit a pair StrDup() / StrFree():
template <typename T>
inline
T* StrDup( const T* str )
{
    if ( ! str ) return NULL;
    T* dup = New TCHAR[ ::_tcslen( str ) + 1 ];
    return ::_tcscpy( dup, str );
}

template <> // specialization for 'char'
inline
CHAR* StrDup< CHAR >( const CHAR* str )
{
    if ( ! str ) return NULL;
    CHAR* dup = New CHAR[ ::strlen( str ) + 1 ];
    return ::strcpy( dup, str );
}

template <typename T>
inline
void StrFree( T& ptr )
{
    safe_delete_array( ptr );
}


Besides, Microsoft’s “safeâ€


Offline
 Profile  
 
 Post subject:
PostPosted: Sun Oct 24, 2010 10:22 pm 

Joined: Sat Aug 16, 2008 7:58 am
Posts: 447
Thank you Marek. I never thought to use an unsigned int...
Now that I see it, it does make sense, since we don't know how big the actuall size of the string will be when using a pointer.


Offline
 Profile  
 
 Post subject:
PostPosted: Mon Oct 25, 2010 12:42 am 

Joined: Sat Aug 16, 2008 7:58 am
Posts: 447
Ok, the only other function I am having trouble converting to the safer version is sscanf, and all of these instances are found in the SceneLib inside the Find Keyword Function

Return_Error Scene::FindKeyword( char *szKeyword, std::string sData, Return_Type retType, void *pData )
{
   // ... more code up here

   // Get The Value(s) Stored In The File
   switch ( retType )
   {
   case RETURN_CHAR:
      {
         char *szName = (char*)pData;
         // Clear Variable Before Using It
         memset( szName, 0, 256 );
         // Get Name, Maximum 255 Characters
         int iNumFound = sscanf( sData.substr( indexA + strlen( szKeyword ) ).c_str(), "%255s", szName );
         if ( iNumFound != 1 )
         {
            // Invalid Name
            return RETURN_NO_VALUE;
         }
         break;
      }
   case RETURN_FLOAT_ARRAY2:
      {
         float *f2Value = (float*)pData;
         int iNumFound = sscanf( sData.substr( indexA + strlen( szKeyword ) ).c_str(), "%f,%f", &f2Value[0], &f2Value[1] );
         if ( iNumFound != 1 )
         {
            // Did Not Find 2 Values For This Keyword
            return RETURN_NO_VALUE;
         }
         break;
      }
   case RETURN_FLOAT_ARRAY3:
      {
         float *f3Value = (float*)pData;
         int iNumFound = sscanf( sData.substr( indexA + strlen( szKeyword ) ).c_str(), "%f,%f,%f", &f3Value[0], &f3Value[1], &f3Value[2] );
         if ( iNumFound != 3 )
         {
            // Did Not Find 3 Values For This Keyword
            return RETURN_NO_VALUE;
         }
         break;
      }
   case RETURN_FLOAT_ARRAY4:
      {
         float *f4Value = (float*)pData;
         int iNumFound = sscanf( sData.substr( indexA + strlen( szKeyword ) ).c_str(), "%f,%f,%f,%f", &f4Value[0], &f4Value[1], &f4Value[2], &f4Value[3] );
         if ( iNumFound != 4 )
         {
            // Did Not Find 4 Values For This Keyword
            return RETURN_NO_VALUE;
         }
         break;
      }
   } // Switch

   //more code down here....



how would I go about to convert the 4 instances of sscanf within this function to sscanf_s ?


Offline
 Profile  
 
 Post subject:
PostPosted: Mon Oct 25, 2010 5:59 am 
Site Admin

Joined: Sun Feb 11, 2007 8:59 am
Posts: 1094
Location: Ontario Canada
If you have a read through this page you'll see the new changes made to this function: http://msdn.microsoft.com/en-us/library/w40768et(VS.80).aspx

Only the case for RETURN_CHAR will need to have its parameters modified, the other three will work fine if you just replace sscanf with sscanf_s.

For the RETURN_CHAR case, you need to add one extra parameter to the end to specify the size of szName. We know the max size of szName is 256 so you can replace your line your code with:

int iNumFound = sscanf_s( sData.substr( indexA + strlen( szKeyword ) ).c_str(), "%255s", szName, 256 );



Offline
 Profile  
 
Display posts from previous:  Sort by  
 Page 1 of 1 [ 12 posts ] 

All times are UTC - 5 hours


Who is online

Users browsing this forum: No registered users and 1 guest


You cannot post new topics in this forum
You cannot reply to topics in this forum
You cannot edit your posts in this forum
You cannot delete your posts in this forum

Jump to:  

cron