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?

Answer 1

Have you tried using Valgrind? That is usually the fastest and easiest way to debug these sorts of errors. If you are reading or writing outside the bounds of allocated memory, it will flag it for you.

Answer 2

malloc can return NULL if no memory is available. You're not checking for that.

Answer 3

@Doug

Have you tried using Valgrind?

Ahhh...figure this was going to be a somewhat platform agnostic issue, but as you bring up Valgrind, I have to now admit that my code is running on a Windows Mobile device (so ARMv4i Architecture).... But I will definitely keep that in mind for my desktop development.

Answer 4

There's a couple of things. You're using sprintf which is inherently unsafe; unless you're 100% positive that you're not going to exceed the size of the buffer, you should almost always prefer snprintf. The same applies to strcat; prefer the safer alternative strncat.

Obviously this may not fix anything, but it goes a long way in helping spot what might otherwise be very annoying to spot bugs.

Answer 5

malloc can return NULL if no memory is available. You're not checking for that.

Right you are... I didn't think about that as I was monitoring the memory and it there was enough free. Is there any way for there to be available memory on the system but for malloc to fail?

Yes, if memory is fragmented. Also, when you say "monitoring memory," there may be something on the system which occasionally consumes a lot of memory and then releases it before you notice. If your call to malloc occurs then, there won't be any memory available. -- Joel

Either way...I will add that check :)

Answer 6

You're using sprintf which is inherently unsafe; unless you're 100% positive that you're not going to exceed the size of the buffer, you should almost always prefer snprintf. The same applies to strcat; prefer the safer alternative strncat.

Yeah..... I mostly do .NET lately and old habits die hard. I likely pulled that code out of something else that was written before my time...

But I'll try not to use those in the future ;)

Answer 7

You know it might not even be your code... Are there any other programs running that could have a memory leak?

Answer 8

@TK

You know it might not even be your code... Are there any other programs running that could have a memory leak?

Probably...and it gets more complicated than that.

The code you see is from a plugin DLL, which is loaded by a service, that is also just a dll, which is loaded by services.exe

And both the service and the plugin are very multi-threaded and there is more than just that plugin loaded. So it's possible...

Answer 9

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.

MSN

Answer 10

It could be your processor. Some CPUs can't address single bytes, and require you to work in words or chunk sizes, or have instructions that can only be used on word or chunk aligned data.

Usually the compiler is made aware of these and works around them, but sometimes you can malloc a region as bytes, and then try to address it as a structure or wider-than-a-byte field, and the compiler won't catch it, but the processor will throw a data exception later.

It wouldn't happen unless you're using an unusual CPU. ARM9 will do that, for example, but i686 won't. I see it's tagged windows mobile, so maybe you do have this CPU issue.

Answer 11

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.

Answer 12

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);