Random segfaults in C++

Question

I'm new to C++ and I haven't a clue where to start, so I uploaded the code to a pastebin because there is a lot of it.

This code compiles fine, and doesn't give off a warning, even with gcc's -Wall option.

It's supposed to generate all prime numbers up to a number given as a command line parameter.

On smaller numbers (e.g. 4,000 or 5,000), it works fine. On larger numbers such as 4,000,000, it crashes with a segfault nearly all the time. On numbers in between, it is hit and miss to whether it runs or not.

Answer 1

You are allocating the array on the stack. The stack is of limited size and can overflow.

Try allocating your array using new/delete.

Answer 2

int primes[max];

Allocating huge arrays in the stack is bad because stacks, by default, have small size. It's better to allocate primes[] in the heap.

Answer 3

Well for one thing, this is problematic:

int sum_array(int num_array[]) {
    int current_total = 0;
    int *iptr = num_array;
    while(*iptr) {
     current_total += *iptr;
     iptr++;
    }
    return current_total;
}

What this says is start at the beginning of the array you are given. While the value in a memory block the size of an int isn't zero, move to the next memory block. What happens here is it'll keep going past the end of your array, eventually messing with memory it shouldn't which is what causes the segfault. The reason it seems random is that sometimes the memory at the end of the array IS empty, and this'll work fine. But sometimes it's not, and then it accesses memory it shouldn't and CRASH.

That might not be the only problem but it was the first one I noticed. To fix it, keep track of the size of your array, pass it to the function, and then use a for loop to iterate up to it rather than using pointers. Like this:

int sum_array(int num_array[], int arraySize) {
    int current_total = 0;
    for(int i = 0; i < arraySize; i++) {
     current_total += num_array[i];
    }
    return current_total;
}

I'll look again to see if there's anything else.

Edit:

On a second look, you do the same thing in two other places. Here:

while(*prime) {
     cout << *prime << " ";
     prime++;
    }

And here:

while(*prime) {
 *prime = 0;
 prime++; 
}

In both places I'd bet dollars to donuts you are overrunning your array and that's what's causing the segfaults. If you're new I'd strongly recommending against using pointer arithmetic to traverse your arrays. Stick to good old for loops and keep track of the end of the for loops.

Other folks have suggested allocating the array from the heap instead of the stack. That's a good idea for a huge array, however, in my experience at least, stack overruns don't usually cause segfaults. The compiler will usually notice when you allocate more space than is in the stack. Still I'd suggesting using a vector (for big extra credit on your homework see if you can figure out how to implement your own vector using a double pointer allocated as an array of pointers from the heap ;) ) or just use the std::vector. It's an expandable array and that will let you add primes to your array as you find them rather than allocating a whole ton of space that you don't necessarily need.

Answer 4

int primes[max];

prime = primes;
while(*prime) {
    *prime = 0;
    prime++; 
}

In the preceding code you can easily run randomly through a memory, Essentially you are going through RAM until you find a 0. If there are no 0s in your array then it will run into memory that doesn't not belong to the process and a segfault will occur.

Edit: As pointed out by Alcon you are doing this in other places in your code too.

Its also best not to allocated primes on the stack as you are unlikely to have that much stack memory available.

To fix this try the following code instead

int primes = new int[max];

size_t count = 0;
while( count < max ) 
{
    prime[count] = 0;
    count++; 
}

And don't forget to call delete[] primes; at the end of your code (when you are finished with the primes array)

Answer 5

You should be dynamically allocating your array of primes, primes, i.e:

int* primes = new int[max];

Your program shouldn't even compile as is. Good luck with your homework!

Answer 6

Others (e.g. Goz) have provided the correct answer -- the values of uninitialised variables cannot be relied on as they vary from run to run. As others have pointed out, it's also risky to allocate large arrays on the stack as stack space is usually more scarce than heap space.

As a side issue, allocating arrays of variable size in the way that you are with primes[max] is not standard-compliant C++, but rather a g++ extension, so your code as it stands is unlikely to work with other compilers. You could use new and delete instead -- but it's even better to get into the habit of using vector<int> in these situations since it will do the cleanup for you.

[EDIT: Thanks Will for pointing out that the real root cause of trouble was elsewhere.]

Answer 7

Goz has pointed out how early things start going wrong.

Its easy for beginners to make mistakes, but even veterans make exactly the same mistakes. For this reason, veterans have made excellent programs for automatically finding the problems:

Free programs include lint, which examines code before you run it, and - even better - valgrind which examines code as you run it! On Windows, there are other commercial alternatives like Purify.

Running programs during development via valgrind and static code checkers, as well as compiling with all warnings enabled from the compiler, is part of development hygiene even for seasoned veterans.