How to prevent copying a wild pointer string

Question

My program is crash intermittently when it tries to copy a character array which is not ended by a NULL terminator('\0').

class CMenuButton {
   TCHAR m_szNode[32];
   CMenuButton() {
     memset(m_szNode, '\0', sizeof(m_szNode));
   }
};
int main() {
  ....
  CString szTemp = ((CMenuButton*)pButton)->m_szNode; // sometime it crashes here
  ...
  return 0;
}

I suspected someone had not copied the character well ended by '\0', and it ended like:

            Stack
m_szNode    $%#^&!&!&!*@*#&!(*@(!*@@&#&*&@@!^&*&#(*!@*((*&*SDFKJSHDF*(&(*&(()(**

Can you tell me what is happening and what should i do to prevent the copying of wild pointer? Help will be very much appreciated!

I guess I'm unable to check if the character array is NULL before copying...

Answer 1

I suspect that your real problem could be that pButton is a bad pointer, so check that out first.

The only way to be 100% sure that a pointer is correct, and points to a correctly sized/allocated object is to never use pointers you didn't create, and never accept/return pointers. You would use cookies, instead, and look up your pointer in some sort of cookie -> pointer lookup (such as a hash table). Basically, don't trust user input.

If you are more concerned with finding bugs, and less about 100% safety against things like buffer overrun attacks, etc. then you can take a less aggressive approach. In your function signatures, where you currently take pointers to arrays, add a size parameter. E.g.:

void someFunction(char* someString);

Becomes

void someFunction(char* someString, size_t size_of_buffer);

Also, force the termination of arrays/strings in your functions. If you hit the end, and it isn't null-terminated, truncate it.

Make it so you can provide the size of the buffer when you call these, rather than calling strlen (or equivalent) on all your arrays before you call them.

This is similar to the approach taken by the "safe string functions" that were created by Microsoft (some of which were proposed for standardization). Not sure if this is the perfect link, but you can google for additional links:

http://msdn.microsoft.com/en-us/library/ff565508(VS.85).aspx

Answer 2

There are two possibilities:

1. pButton doesn't point to a CMenuButton like you think it does, and the cast is causing undefined behavior.
2. The code that sets m_szNode is incorrect, overflowing the given size of 32 characters.

Since you haven't shown us either piece of code, it's difficult to see what's wrong. Your initialization of m_szNode looks OK.

Is there any reason that you didn't choose a CString for m_szNode?

Answer 3

My approach would be to make m_szNode a private member in CMenuButton, and explicitly NULL-terminate it in the mutator method.

class CMenuButton {
  private:
    TCHAR m_szNode[32];

  public:
    void set_szNode( TCHAR x ) {
        // set m_szNode appropriately
        m_szNode[ 31 ] = 0;
     }
};