Segmentation fault from pointer error (simple...)

Question

I have a function which returns a pointer to an array of doubles:

double * centerOfMass(System &system) {
        long unsigned int size = system.atoms.size();

        double x_mass_sum=0.0; double y_mass_sum=0.0; double z_mass_sum=0.0; double mass_sum=0.0;

        for (int i=0; i<=size; i++) {
                double atom_mass = system.atoms[i].m;
                mass_sum += atom_mass;

                x_mass_sum += system.atoms[i].pos["x"]*atom_mass;
                y_mass_sum += system.atoms[i].pos["y"]*atom_mass;
                z_mass_sum += system.atoms[i].pos["z"]*atom_mass;
        }

        double comx = x_mass_sum/mass_sum;
        double comy = y_mass_sum/mass_sum;
        double comz = z_mass_sum/mass_sum;

        double* output = new double[3];  // <-------- here is output
        output[0] = comx*1e10; // convert all to A for writing xyz
        output[1] = comy*1e10;
        output[2] = comz*1e10;
        return output;
}

When I try to access the output by saving the array to a variable (in a different function), I get a segmentation fault when the program runs (but it compiles fine):

void writeXYZ(System &system, string filename, int step) {

        ofstream myfile;
        myfile.open (filename, ios_base::app);
        long unsigned int size = system.atoms.size();
        myfile << to_string(size) + "\nStep count: " + to_string(step) + "\n";

        for (int i = 0; i < size; i++) {
                myfile << system.atoms[i].name;
                myfile <<  "   ";
                myfile << system.atoms[i].pos["x"]*1e10;
                myfile <<  "   ";
                myfile << system.atoms[i].pos["y"]*1e10;
                myfile <<  "   ";
                myfile << system.atoms[i].pos["z"]*1e10;
                myfile << "\n";
        }

        // get center of mass
        double* comfinal = new double[3]; // goes fine
        comfinal = centerOfMass(system); // does NOT go fine..
        myfile << "COM   " << to_string(comfinal[0]) << "   " << to_string(comfinal[1]) << "   " << to_string(comfinal[2]) << "\n";

        myfile.close();
}

Running the program yields normal function until it tries to call centerOfMass.

I've checked most possible solutions; I think I just lack understanding on pointers and their scope in C++. I'm seasoned in PHP so dealing with memory explicitly is problematic.

Thank you kindly

`for (int i=0; i<=size; i++) {` seems weird. Shouldn't it be `for (int i=0; i — songyuanyao, Jan 21 '17 at 03:07
@songyuanyao shoot dang, thank you. Missed that. Thanks for your eyes. If you post an answer I'll accept :3 — khaverim, Jan 21 '17 at 03:09
@khaverim Good news--you don't necessarily misunderstand pointers and their scope in C++. You just made a simple error, which is okay. — synchronizer, Jan 21 '17 at 03:14
You have two memory leaks in your program. You initialize `comfinal` here `double* comfinal = new double[3];` and then re-assign the pointer in this line `comfinal = centerOfMass(system);` When you perform the second assignment you leak the memory you allocated in your first call to new. Then at the end of your program you fail to call delete on `comfinal` thus leaking the memory it held at the end of the program. — Alex Zywicki, Jan 21 '17 at 03:18
@AlexZywicki I call `std::exit(0);` at the end of my `main` function. Doesn't that clear things up? — khaverim, Jan 21 '17 at 03:22
@khaverim no it does not, it actually in many cases can make things worse. You need to change these two lines `double* comfinal = new double[3]; comfinal = centerOfMass(system);` into `double* comfinal = centerOfMass(system);` and then at the end of `writeXYZ` add `delete[] comfinal` — Alex Zywicki, Jan 21 '17 at 03:31
@khaverim see this link about using `std::exit` https://stackoverflow.com/questions/461449/return-statement-vs-exit-in-main — Alex Zywicki, Jan 21 '17 at 03:32

songyuanyao · Accepted Answer · 2017-01-21T03:58:22.080

I'm not sure about the type of system.atoms. If it's a STL container like std::vector, the condition part of the for loop inside function centerOfMass is wrong.

long unsigned int size = system.atoms.size();
for (int i=0; i<=size; i++) {

should be

long unsigned int size = system.atoms.size();
for (int i=0; i<size; i++) {

PS1: You can use Range-based for loop (since C++11) to avoid such kind of problem.

PS2: You didn't delete[] the dynamically allocated arrays; Consider about using std::vector, std::array, or std::unique_ptr instead, they're designed to help you to avoid such kind of issues.

score 1 · Answer 2 · answered Jan 21 '17 at 03:55

In addition to the concerns pointed out by songyuanyao, the usage of the function in writeXYZ() causes a memory leak.

To see this, note that centerOfMass() does (with extraneous details removed)

 double* output = new double[3];  // <-------- here is output
 // assign values to output
 return output;

and writeXYZ() does (note that I've changed comments to reflect what is actually happening, as distinct from your comments on what you thought was happening)

double* comfinal = new double[3]; //  allocate three doubles
comfinal = centerOfMass(system); //   lose reference to them

// output to myfile

If writeXYZ() is being called multiple times, then the three doubles will be leaked every time, EVEN IF, somewhere, delete [] comfinal is subsequently performed. If this function is called numerous times (for example, in a loop) eventually the amount of memory leaked can exceed what is available, and subsequent allocations will fail.

One fix of this problem is to change the relevant part of writeXYZ() to

double* comfinal = centerOfMass(system);

// output to myfile

delete [] comfinal;    // eventually

Introducing std::unique_ptr in the above will alleviate the symptoms, but that is more a happy accident than good logic in the code (allocating memory only to discard it immediately without having used it is rarely good technique).

In practice, you are better off using standard containers (std::vector, etc) and avoid using operator new at all. But they still require you to keep within bounds.

Segmentation fault from pointer error (simple...)

2 Answers2