Why does strcpy trigger a segmentation fault with global variables?

Question

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.

Solution 1

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.

Solution 2

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.

Solution 3

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

Solution 4

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

Solution 5

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

View more solutions

Comments

[Serafina Brocious]

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.

[Serafina Brocious]

In addition, it needs to be at least strlen(path)+1 to fit the null term or Bad Things T(M) will result.

[hunt]

Adam, is it a good idea to have all those magic numbers floating around? ;-)

[Torlack]

that still has bugs since if the source string is of 256 length, then the strings don't have a null termination.

[John Millikin]

He means you don't need the "temp[0] = 0" line, since strcpy() will add the NULL terminator for you.

[Torlack]

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)

[terminus]

Sorry, misunderstood 'strcpy' for 'strcat'. I fixed my answer

[Torlack]

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.

[codemonkey]

As expected, swapping the order of the variable declarations makes the program segfault. Thanks!

[bloodphp]

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.

[bloodphp]

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.

[Adam Rosenfield]

@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.