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 sounds like you're using print statements to debug segmentation faults: a big no-no in C.

The problem is that stdout is buffered on most systems, which means that the segmentation fault is actually occurring later than you think. It is impossible to reliably determine out when your program is segfaulting using printf.

Instead, you should use a debugger like gdb, which will tell you the exact line of code that is causing the segmentation fault.

If you don't know how to use gdb, here's a quick tutorial I found by Google'ing: http://www.cs.cmu.edu/~gilpin/tutorial/

Answer 3

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

Answer 5

This is odd:

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

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

You allocate memory and then you free it right afterwards and leaving your pointers pointing to unallocated memory.

This line looks also dangerous:

atoms[bonds[i] - totRead].type[0] == 'H'

Make sure you stay inside the array with your index.

Answer 6

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 7

Here's a small rewrite of parts of your code to demonstrate the memory leaks:

atoms = (struct Atom *) malloc(sizeof(struct Atom) * numAtoms); // allocating new struct
atoms = (*amino).atoms; // overriding the pointer with pointer to a struct allocated in the caller
//...
for (some counter on atoms)
{
    if (something that succeeds)
    {
      atoms = (struct Atom *) malloc(sizeof(struct Atom)); // overwrite the pointer yet again with newly allocated struct
      free(atoms); // free the last allocated struct
      // at this point atoms points to invalid memory, so on the next iteration of the outer for it'll crash
    }
}

There's also chance that the statement bonds[i] - totRead can be out of the atoms[] bounds, which could be the segfault.

Answer 8

Where you have written:

/* Allocate new space for a copy of the input parameter "Atoms" */
atoms = (struct Atom *) malloc(sizeof(struct Atom) * numAtoms);
/* Immediately lose track of the pointer to that space, once was stored
   in atoms, now being lost. */
atoms = (*amino).atoms;

I think your intention must be this:

/* Allocate new space for a copy of the input parameter "Atoms" */
atoms = (struct Atom *)malloc(sizeof(struct Atom) * numAtoms);

/* Copy the input parameter into the newly-allocated memory. */
for (i = 0; i < numAtoms; i++)
    atoms[i] = (*amino).atoms[i];

which could also be written as:

/* Allocate new space for a copy of the input parameter "Atoms" */
atoms = (struct Atom *)malloc(sizeof(struct Atom) * numAtoms);

/* Copy the input parameter into the newly-allocated memory. */
memcpy(atoms, (*amino).atoms, sizeof(struct Atom) * numAtoms);

In C there is no built-in equals (=) operator to copy arrays as you seem to have intended. What you have instead loses track of the pointer to allocated memory, formerly stored in the variable atoms, and then proceeds to begin the first iteration of your loop with atoms pointing into the "input copy" of the atoms array.

---

Part of the problem is that you are calling free on memory, but then subsequently you continue to access the pointer to this freed memory. You must not access pointers to freed memory. To avoid this, replace all of your calls to free with:

#ifdef free
#    undef free
#endif
#define free(f) freeptr(&f)

void freeptr(void **f)
{
    /* This function intentionally segfaults if passed f==NULL, to alert
       the programmer that an error has been made.  Do not wrap this code
       with a check "if (f==NULL)", fix the problem where it is called. 

       To pass (*f==NULL) is a harmless 'no-op' as per the C standard
       free() function.

       If you must access the original, built-in free(), use (free)() to
       bypass the #define macro replacement of free().

     */

    (free)(*f);  /* free() must accept NULL happilly; this is safe. */
    *f = NULL;   /* Set the pointer to NULL, it cannot be used again. */
}

For now you can simply cut-and-paste the above code somewhere at the top of your program. A good place is after the ultimate #include directive, but it must occur at file-level scope and prior to your first use of free() in the code.

After recompiling your code, you will find Bus Errors and Segmentation Violation faults immediately after you free(atom). This is correct and the purpose of freeptr() is to lead your code to an immediate crash rather than the current situation where your code is misusing pointers and leading to problems which are very difficult for you to debug.

To finally correct your memory allocations, definitely transpose the lines:

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

which should read:

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

---

You use the argument diff as though you are passing in an array of at least three (3) elements. You should verify that the caller is passing enough memory.

---

When allocating bonds, you must allocate memory for not one (1) integer, but as many integers as numBonds:

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

or, better for most C coders:

free(bonds);
/* The calloc function performs the multiplication internally, and
   nicely zero-fills the allocated memory. */
bonds = calloc(numBonds, sizeof(int));

You will need to correct the allocation of atoms to allocate a correct number of elements. Currently, you also are allocating only a single memory element of size sizeof(struct Atom). An array of such elements requires that you multiply the size of one element by the number of elements.

The calloc() function is nice because it allocates an array for you, and initializes the content of all elements to zero. malloc() does nothing to initialize the returned memory, and can result in unpredictable values propagating in your program. If you use malloc() rather than calloc(), you must take care to initialize the array elements. Even when using calloc(), you must initialize any non-zero elements.

---

Notice that I removed the cast from the return value of malloc. If you are writing C code, you should be compiling it as C code. The compiler will not complain about the lack of a cast from void * unless you are compiling in a C++ mode. C source files should end in .c file extensions, not .cpp.

---

As Walter Mundt pointed out, you are accidentally calling free() on a member of one of your input parameters, which you have assigned to the pointer atoms. You will have to correct this on your own; the above freeptr() will not highlight this mistake for you.

---

Others have written that you cannot use printf() to reliably detect where your program is crashing. The output from printf() is buffered and its appearance is delayed.

Your best bet is to use gdb to determine the exact line at which your program crashes. You won't have to learn any gdb commands to do this if you compile your code for debugging.

Lacking that, replace:

printf("Program ran to point A.\n");

with:

fprintf(stderr, "Program ran to point A.\nPress return.\n");
fflush(stderr); /* Force the output */
fflush(stdin);  /* Discard previously-typed keyboard input */
fgetc(stdin);   /* Await new input */
fflush(stdin);  /* Discard unprocessed input */

---

Overall, my suggestion is that you not use the C language for the time being. Computers are so fast these days that I would question why you have considered C in the first place.

Don't get me wrong; I love the C language. But C is not for everything. C is great for operating systems, embedded systems, high-performance computing, and for other cases where the main obstacle to success is lack of low-level access to the computing machinery.

In your case, you seem to be a scientist or engineer. I recommend you learn and use Python. Python lends itself to easily read, easily verified programs which you can share with your fellow chemists or engineers. C does not lend itself to quickly writing robust code as Python does. In that unlikely future event that Python is not fast enough for your purposes, there are other solutions which you will then be ready for.

Answer 9

Do you have an #include <stdio.h> in the file? I wonder if you're getting a call to printf() that's using an implicit declaration of printf() and therefore might be using the wrong calling convention.

What's the compiler/platform you're using? Do you get any warnings from the build?

Answer 10

EDIT: go read this http://stackoverflow.com/questions/233148/c-pointers-and-arrays-question it should help you understand what pointers and arrays are

These lines don't actually have any real net effect on what the code does so you can remove them

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

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

The malloc lines here are useless and result in a leak since you assign the pointer from the amino struct to atoms and bonds right after doing it.

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;

You should stop coding for a bit and make sure you understand pointers before you do much else because you clearly don't and it's just going to cause you lots and lots of pain until you do, however here is a version of your code that should do something more like what you want:

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

    struct Atom* atoms;
    int* bonds;
    int numBonds;
    int i;
    int retVal;
    int numAtoms = amino->numAtoms;

    numAtoms = amino->numAtoms;
    atoms = amino->atoms;

    numBonds = atoms[nPos].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;

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

            return retVal;
        }
}