memset() causing data abort

Question!

I'm getting some strange, intermittent, data aborts (< 5% of the time) in some of my code, when calling memset. The problem is that is usually doesn't happen unless the code is running for a couple days, so it's hard to catch it in the act.

I'm using the following code:

char *msg = (char*)malloc(sizeof(char)*2048);
char *temp = (char*)malloc(sizeof(char)*1024);
memset(msg, 0, 2048);
memset(temp, 0, 1024);
char *tempstr = (char*)malloc(sizeof(char)*128);

sprintf(temp, "%s %s/%s %s%s", EZMPPOST, EZMPTAG, EZMPVER, TYPETXT, EOL);
strcat(msg, temp);

//Add Data
memset(tempstr, '\0', 128);
wcstombs(tempstr, gdevID, wcslen(gdevID));
sprintf(temp, "%s: %s%s", "DeviceID", tempstr, EOL);
strcat(msg, temp);

As you can see, I'm not trying to use memset with a size larger that what's originally allocated with malloc()

Anyone see what might be wrong with this?



Answers

wcstombs doesn't get the size of the destination, so it can, in theory, buffer overflow.

And why are you using sprintf with what I assume are constants? Just use:

EZMPPOST" " EZMPTAG "/" EZMPVER " " TYPETXT EOL

C and C++ combines string literal declarations into a single string.



NB borrowed some comments from other answers and integrated into a whole. The code is all mine...

  • Check your error codes. E.g. malloc can return NULL if no memory is available. This could be causing your data abort.
  • sizeof(char) is 1 by definition
  • Use snprintf not sprintf to avoid buffer overruns
    • If EZMPPOST etc are constants, then you don't need a format string, you can just combined several string literals as STRING1 " " STRING2 " " STRING3 and strcat the whole lot.
  • You are using much more memory than you need to.
  • With one minor change, you don't need to call memset in the first place. Nothing really requires zero initialisation here.

This code does the same thing, safely, runs faster, and uses less memory.

    // sizeof(char) is 1 by definition. This memory does not require zero
    // initialisation. If it did, I'd use calloc.
    const int max_msg = 2048;
    char *msg     = (char*)malloc(max_msg);
    if(!msg)
    {
       // Allocaton failure
       return;
    }
    // Use snprintf instead of sprintf to avoid buffer overruns
    // we write directly to msg, instead of using a temporary buffer and then calling
    // strcat. This saves CPU time, saves the temporary buffer, and removes the need
    // to zero initialise msg.
    snprintf(msg, max_msg, "%s %s/%s %s%s", EZMPPOST, EZMPTAG, EZMPVER, TYPETXT, EOL);

   //Add Data
   size_t len = wcslen(gdevID);
   // No need to zero init this
   char* temp = (char*)malloc(len);
   if(!temp)
   {
      free(msg);
      return;
   }
   wcstombs(temp, gdevID, len);
   // No need to use a temporary buffer - just append directly to the msg, protecting 
   // against buffer overruns.
   snprintf(msg + strlen(msg), 
           max_msg - strlen(msg), "%s: %s%s", "DeviceID", temp, EOL);
   free(temp);


Instead of doing malloc followed by memset, you should be using calloc which will clear the newly allocated memory for you. Other than that, do what Joel said.



This video can help you solving your question :)
By: admin