C segmentation fault before/during return statement

Question

I print the value that I'm returning right before my return statement, and tell my code to print the value that was returned right after the function call. However, I get a segmentation fault after my first print statement and before my second (also interesting to note, this always happens on the third time the function is called; never the first or the second, never fourth or later). I tried printing out all of the data that I'm working on to see if the rest of my code was doing something it maybe shouldn't, but my data up to that point looks fine. Here's the function:

int findHydrogen(struct Amino* amino, int nPos, float* diff, int totRead) {

    struct Atom* atoms;
    int* bonds;
    int numBonds;
    int i;
    int retVal;
    int numAtoms;

    numAtoms = (*amino).numAtoms;

    atoms = (struct Atom *) malloc(sizeof(struct Atom) * numAtoms);
    atoms = (*amino).atoms;

    numBonds = atoms[nPos].numBonds;

    bonds = (int *) malloc(sizeof(int) * numBonds);
    bonds = atoms[nPos].bonds;

    for(i = 0; i < (*amino).numAtoms; i++)
        printf("ATOM\t\t%d  %s\t0001\t%f\t%f\t%f\n", i + 1, atoms[i].type, atoms[i].x, atoms[i].y, atoms[i].z);

    for(i = 0; i < numBonds; i++) 
        if(atoms[bonds[i] - totRead].type[0] == 'H') {
            diff[0] = atoms[bonds[i] - totRead].x - atoms[nPos].x;
            diff[1] = atoms[bonds[i] - totRead].y - atoms[nPos].y;
            diff[2] = atoms[bonds[i] - totRead].z - atoms[nPos].z;

            retVal = bonds[i] - totRead;

            bonds = (int *) malloc(sizeof(int));
            free(bonds);

            atoms = (struct Atom *) malloc(sizeof(struct Atom));
            free(atoms);

            printf("2 %d\n", retVal);

            return retVal;
        }
}

As I mentioned before, it works fine the first two times I run it, the third time it prints the correct value of retVal, then seg faults somewhere before it gets to where I called the function, which I do as:

hPos = findHydrogen((&aminoAcid[i]), nPos, diff, totRead);
printf("%d\n", hPos);

Answer 1

A segmentation fault while returning is normally an indication of a mangled stack.

Answer 2

It's not easy to guess where the error is from this code(there's a potential for bug in just about every line of code here) - likely you have a buffer overrun somewhere, however if you're on a *nix , run your program under valgrind, you should be able to find the error rather quickly.

These lines look odd though:

atoms = (struct Atom *) malloc(sizeof(struct Atom) * numAtoms);
atoms = (*amino).atoms;

You're leaking memory, as you discard the pointer returned by malloc. Same thing with bonds, and same thing over again inside your for loop.

Answer 3

EDIT Well, you're leaking memory left and right, but not quite in the way I was thinking. Fixed sequence below:

Specifically, when you do:

atoms = (struct Atom *) malloc(sizeof(struct Atom) * numAtoms); // 1
atoms = (*amino).atoms; // 2
// ...
atoms = (struct Atom *) malloc(sizeof(struct Atom)); // 3
free(atoms); // 4

What is happening is that you are allocating some memory and putting the address in atoms in step(1). Then, you toss away that address and instead point atoms at part of the inside of your amino structure in (2). Then you allocate a second pointer with a single atom. Finally, you call free on that. You treat bonds the same way. You probably mean something like this:

atoms = (struct Atom *) malloc(sizeof(struct Atom) * numAtoms); // 1
memcpy(atoms, (*amino).atoms, sizeof(struct Atom) * numAtoms); // 2
// ...
// delete 3
free(atoms); // 4

Note that if an Atom has any pointer components you might want to do a for loop and copy the atoms individually along with their contents, which you would then have to individually free at the return point.

...or maybe just this if you are only wanting to read the atoms data from the structure:

atoms = (*amino).atoms; // delete 1, 3, 4 entirely and just read directly from the structure

Other answers talking about the amount of space in diff and other issues are probably also worth investigating.

EDIT: fixed the sequence of calls to match the code sample.

Answer 4

There are a lot of things wrong here.

The first thing I notice is that you're leaking memory (you allocate some memory at (struct Atom *) malloc(sizeof(struct Atom) * numAtoms), then overwrite the pointer with the pointer in the amino structure); you do the same thing with (int *) malloc(sizeof(int) * numBonds); .

Second, you're not bounds-checking the expression bonds[i] - totRead.

Third, and I think this is where you're crashing, you overwrite your atoms pointer here: atoms = (struct Atom *) malloc(sizeof(struct Atom)); free(atoms); which leaves atoms pointing to invalid memory.