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<size; i++) { ? What's the type of system.atoms? — songyuanyao
– songyuanyao, Commented Jan 21, 2017 at 3:07
@songyuanyao shoot dang, thank you. Missed that. Thanks for your eyes. If you post an answer I'll accept :3 — khaverim
– khaverim, Commented Jan 21, 2017 at 3: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
– synchronizer, Commented Jan 21, 2017 at 3: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
– Alex Zywicki, Commented Jan 21, 2017 at 3:18
@AlexZywicki I call std::exit(0); at the end of my main function. Doesn't that clear things up? — khaverim
– khaverim, Commented Jan 21, 2017 at 3:22

songyuanyao · Accepted Answer · 2017-01-21 03:58:22Z

4

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.

edited Jan 21, 2017 at 3:58

answered Jan 21, 2017 at 3:12

songyuanyao

174k16 gold badges327 silver badges432 bronze badges

Sign up to request clarification or add additional context in comments.

Comments

Peter · Accepted Answer · 2017-01-21 03:55:08Z

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.

Collectives™ on Stack Overflow

Segmentation fault from pointer error (simple...)

2 Answers 2

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related