Memset () causes data interruption

Question

I get some weird intermittent data interrupts (<5% of the time) in some of my code when memset() called. The problem is that usually this does not happen if the code does not work for a couple of days, so it is difficult to catch it in action.

I am 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 am not trying to use a memset with a size larger than what was originally allocated using malloc()

Can anyone understand what could be wrong with this?

Answer 1

malloc can return NULL if there is no memory. You are not checking this.

Answer 2

There are a few things. You are using sprintf , which is inherently unsafe; if you are not 100% sure that you are not going to exceed the buffer size, you almost always prefer snprintf . The same applies to strcat ; prefer a safer alternative to strncat .

Obviously, this may not fix anything, but it is a long way to help determine that otherwise it can be very unpleasant to detect errors.

Answer 3

malloc may return NULL if memory is not available. You do not check that.

You are right ... I didn’t think about it, since I controlled the memory, and that was enough. Is there a way for there to be available memory on the system, but for malloc failure?

Yes, if the memory is fragmented. Also, when you say “memory monitoring,” there may be something in the system that occasionally consumes a lot of memory, and then releases it before you notice it. If your malloc call occurs, then there will be no available memory. - Joel

Anyway ... I will add that check :)

Answer 4

wcstombs does not get the size of the destination, so theoretically it could be buffer overflowed.

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

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

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

Answer 5

Have you tried using Valgrind? This is usually the fastest and easiest way to debug such errors. If you read or write outside the allocated memory, it will mark this for you.

Answer 6

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

Yes ..... I mainly do .NET recently, and old habits die hard. I probably pulled this code from something else written before my time ...

But I will try not to use them in the future;)

Answer 7

You know that this may not even be your code ... Are there other programs that might have a memory leak?

Answer 8

It could be your processor. Some processors cannot address single bytes and require that you work in words or block sizes or have instructions that can only be used for data aligned by word or block.

Usually the compiler finds out about it and works around them, but sometimes you can malloc a region as bytes, and then try to access it as a structure or wider than a byte, and the compiler won't catch it, but the processor will throw a data exception later.

This will not happen if you are not using an unusual processor. ARM9 will do this, for example, but the i686 will not. I see that these are labeled windows for mobile devices, so maybe you have this problem with the processor.

Answer 9

Instead of malloc , followed by memset , you should use calloc , which will clear the memory you have allocated. Other than that, do what Joel said.

Answer 10

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

• Check for error codes. For instance. malloc can return NULL if there is no memory. This can lead to interruption of your data.
• sizeof (char) - 1 by definition
• Use snprintf rather than sprintf to avoid buffer overflows
  • If EZMPPOST etc. are constants, then you do not need a format string, you can simply combine several string literals like STRING1 "STRING2" "STRING3 and strcat of the entire batch.
• You are using a lot more memory than you need.
• With one minor change, you do not need to call memset first. Nothing really requires zero initialization.

This code does the same, is safe, 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);