views:

2301

answers:

8

So I've got some C code:

#include <stdio.h>
#include <string.h>

/* putting one of the "char*"s here causes a segfault */
void main() {
  char* path = "/temp";
  char* temp;
  strcpy(temp, path);
}

This compiles, runs, and behaves as it looks. However, if one or both of the character pointers is declared as global variable, strcpy results in a segmentation fault. Why does this happen? Evidently there's an error in my understanding of scope.

+7  A: 

You forgot to allocate and initialize temp:

temp = (char *)malloc(TEMP_SIZE);

Just make sure TEMP_SIZE is big enough. You can also calculate this at run-time, then make sure the size is enough (should be at least strlen(path))

terminus
There's no need to initialize the memory at temp -- strcpy will take care of this, even if it's just setting the initial null term.
Cody Brocious
In addition, it needs to be at least strlen(path)+1 to fit the null term or Bad Things T(M) will result.
Cody Brocious
He means you don't need the "temp[0] = 0" line, since strcpy() will add the NULL terminator for you.
John Millikin
strcpy doesn't care what the destination string points to as long as it is large enough to contain the result. The temp[0] = 0 is pointless and even if the source string is empty, will still be set by strcpy. If you want to clear the whole string, you have to use memset (or something else)
Torlack
Sorry, misunderstood 'strcpy' for 'strcat'. I fixed my answer
terminus
No need to cast the pointer returned by malloc -- malloc returns a void pointer and thus no cast is needed. In addition, casting it could hurt you because it could hide problems with malloc not being properly defined (as the implicit definition of malloc is it returning an int) at this line of code.
Daniel Papasian
+1  A: 

The important part to note:
destination string dest must be large enough to receive the copy.
In your situation temp has no memory allocated to copy into.

Copied from the man page of strcpy:

DESCRIPTION
   The  strcpy()  function  copies the string pointed to by src (including
   the terminating '\0' character) to the array pointed to by  dest.   The
   strings  may not overlap, and the destination string dest must be large
   enough to receive the copy.
Martin York
+8  A: 

The temp variable doesn't point to any storage (memory) and it is uninitialized.

if temp is declared as char temp[32]; then the code would work no matter where it is declared. However, there are other problems with declaring temp with a fixed size like that, but that is a question for another day.

Now, why does it crash when declared globally and not locally. Luck...

When declared locally, the value of temp is coming from what ever value might be on the stack at that time. It is luck that it points to an address that doesn't cause a crash. However, it is trashing memory used by someone else.

When declared globally, on most processors these variables will be stored in data segments that will use demand zero pages. Thus char *temp appears as if it was declared char *temp=0.

Torlack
+1  A: 

You're invoking undefined behavior, since you're not initializing the temp variable. It points to a random location in memory, so your program may work, but most likely it will segfault. You need to have your destination string be an array, or have it point to dynamic memory:

// Make temp a static array of 256 chars
char temp[256];
strncpy(temp, 256, path);

// Or, use dynamic memory
char *temp = (char *)malloc(256);
strncpy(temp, 256, path);

Also, use strncpy() instead of strcpy() to avoid buffer overruns.

Adam Rosenfield
Adam, is it a good idea to have all those magic numbers floating around? ;-)
Rob Wells
that still has bugs since if the source string is of 256 length, then the strings don't have a null termination.
Torlack
No need to cast the pointer returned by malloc -- malloc returns a void pointer and thus no cast is needed. In addition, casting it could hurt you because it could hide problems with malloc not being properly defined (as the implicit definition of malloc is it returning an int) at this line of code.
Daniel Papasian
@Daniel Papasian: the cast is not necessary in C, but it is necessary in C++, and I usually like to make my C code as compatible with C++ as possible. The use of implicitly defined functions without declaring them is deprecated in C, and they should not be used in new C code. I always compile with -Wall, which includes -Wimplicit-function-declaration.
Adam Rosenfield
+3  A: 

As mentioned above, you forgot to allocate space for temp. I prefer strdup to malloc+strcpy. It does what you want to do.

Sridhar Iyer
I didn't know about strdup. Thanks.
Grant
+2  A: 

No - this doesn't work regardless of the variables - it just looks like it did because you got (un)lucky. You need to allocate space to store the contents of the string, rather than leave the variable uninitialised.

Uninitialised variables on the stack are going to be pointing at pretty much random locations of memory. If these addresses happen to be valid, your code will trample all over whatever was there, but you won't get an error (but may get nasty memory corruption related bugs elsewhere in your code).

Globals consistently fail because they usually get set to specific patterns that point to unmapped memory. Attempting to dereference these gives you an segfault immediately (which is better - leaving it to later makes the bug very hard to track down).

Brian
+10  A: 

As other posters mentioned, the root of the problem is that temp is uninitialized. When declared as an automatic variable on the stack it will contain whatever garbage happens to be in that memory location. Apparently for the compiler+CPU+OS you are running, the garbage at that location is a valid pointer. The strcpy "succeeds" in that it does not segfault, but really it copied a string to some arbitrary location elsewhere in memory. This kind of memory corruption problem strikes fear into the hearts of C programmers everywhere as it is extraordinarily difficult to debug.

When you move the temp variable declaration to global scope, it is placed in the BSS section and automatically zeroed. Attempts to dereference *temp then result in a segfault.

When you move *path to global scope, then *temp moves up one location on the stack. The garbage at that location is apparently not a valid pointer, and so dereferencing *temp results in a segfault.

DGentry
As expected, swapping the order of the variable declarations makes the program segfault. Thanks!
Grant
+2  A: 

I'd like to rewrite first Adam's fragment as

// Make temp a static array of 256 chars
char temp[256];
strncpy(temp, sizeof(temp), path);
temp[sizeof(temp)-1] = '\0';

That way you:

1. don't have magic numbers laced through the code, and
2. you guarantee that your string is null terminated.

The second point is at the loss of the last char of your source string if it is >=256 characters long.

Rob Wells
When using wchar_t, this doesn't work. Using sizeof only works with chars. I know, this is a question about chars, but I run into a lot of bugs where people use sizeof with wchar_t.
Torlack