Segmentation error before / during the return operation

Question

I print the value that I return right before my return statement, and tell my code to print the value that was returned immediately after the function call. However, I get a segmentation error after my first print statement and before my second (it’s also interesting to note that this always happens the third time the function is called, never the first or second, never the fourth or later). I tried to print all the data I'm working on to see if the rest of my code is doing something that might not be, but my data looks fine up to this point. 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 earlier, it works great the first two times when I run it, the third time it prints the correct retVal value, and then the seg errors somewhere before it gets to where I called the function that I execute as:

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

Answer 1

It’s not easy to guess where the error is from this code (there is a chance of error in almost every line of code) - most likely you have a buffer overflow somewhere, however if you are in * nix, run your program under valgrind , you should quickly find the error .

These lines look weird:

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

You are losing memory as you discard the pointer returned by malloc. Same thing with bonds , and the same thing again inside the for loop.

Answer 2

A segmentation error on return is usually a sign of a garbled stack.

Answer 3

EDIT. Well, you lose memory left and right, but not quite as I thought. The fixed sequence is below:

In particular, 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 happens is that you allocate some memory and put the address in atoms in step (1). Then you drop that address and instead specify atoms in the internal part of your amino structure in (2). Then you select the second pointer with one atom. Finally, you call free . 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 

Please note that if Atom has any pointer components, you can make a for loop and copy the atoms separately along with their contents, which then you will need separately free at the return point.

... or maybe just this if you only want to read the atom data from the structure:

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

Other answers regarding the number of spaces in diff and other issues are also worth exploring.

EDIT: fixed call sequence to match sample code.

Answer 4

There are a lot of things wrong.

The first thing I notice is that you skip memory (you allocate some memory in (struct Atom *) malloc(sizeof(struct Atom) * numAtoms) , and then overwrite the pointer with a pointer in the s structure); you do the same with (int *) malloc(sizeof(int) * numBonds); .

Secondly, you are not limited - by checking the expression bonds[i] - totRead .

Thirdly, and I think this is where you crash, you rewrite the pointer to atoms here: atoms = (struct Atom *) malloc(sizeof(struct Atom)); free(atoms); atoms = (struct Atom *) malloc(sizeof(struct Atom)); free(atoms); , which leaves atoms indicating invalid memory.

Answer 5

Here's a little rewrite of parts of your code to demonstrate 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 } } 

It is also likely that the operator bonds[i] - totRead may be outside the boundaries of atoms[] , which may be segfault.

Answer 6

It looks like you are using print statements to debug segmentation errors: big no-no in C.

The problem is that stdout is buffered on most systems, which means that the segmentation error actually occurs later than you think. It is not possible to reliably determine when your program was interrupted with printf.

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

If you don’t know how to use gdb, here is a quick tutorial I found on Google: http://www.cs.cmu.edu/~gilpin/tutorial/

Answer 7

This is odd:

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

You allocate memory, and then free it immediately and leave pointers pointing to unallocated memory.

This line also looks dangerous:

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

Make sure you stay inside the array with the index.

Answer 8

EDIT: go read this Accessing array values ​​using pointer arithmetic and signature in C , this should help you understand which pointers and arrays

These lines actually have no real effect on what the code does to 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 lead to a leak, because you assign a pointer from the structure of the amino group to atoms and bonds immediately afterwards.

 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 need to stop coding a bit and make sure that you understand the pointers before doing something else, because you obviously don’t do this, and it will just cause you a lot and pain while you do it, but here is the version of your code, which 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; } } 

Answer 9

Where did you write:

 /* 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 should be like 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 can 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); 

There is no built-in equals ( = ) operator in C for copying arrays, as you seem intended. Instead, you lose track of the pointer to the allocated memory previously stored in the atoms variable, and then proceed to the start of the first iteration of your loop with atoms , pointing to the "input copy" of the atom array.

---

Part of the problem is that you call free in memory, but then you continue to access the pointer to that freed memory. You should not access pointers to freed memory. To avoid this, replace all your calls with a free one:

 #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. */ } 

Now you can simply cut and paste the above code somewhere at the top of your program. A good place after the final #include directive, but it should happen in the file level area and before your first use of free() in your code.

After recompiling the code, you will find errors Bus errors and segmentation violations immediately after free(atom) . This is correct, and the goal of freeptr() is to cause your code to crash immediately, and not to the current situation where your code uses pointers incorrectly and leads to problems that are very difficult for you to debug.

To permanently fix memory allocation, definitely transpose the lines:

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

which should read:

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

---

You use the diff argument as if you were passing an array of at least three (3) elements. You must make sure that the caller has enough memory.

---

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

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

or, better for most C encoders:

 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 adjust the selection of atoms to select the correct number of elements. Currently, you also allocate only one 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 good because it allocates an array for you and initializes the contents of all elements to zero. malloc() does nothing to initialize the returned memory and may lead to unpredictable values ​​propagating in your program. If you use malloc() rather than calloc() , you must take care of initializing the elements of the array. Even when using calloc() you must initialize any nonzero elements.

---

Note that I removed the listing from the malloc return value. If you are writing C code, you must compile it as C code. The compiler will not complain about the lack of casting from void * unless you compile in C ++ mode. C source files should end with .c file extensions, not .cpp .

---

As Walter Mundt noted, you accidentally call free() on a member of one of your input parameters that you assigned to the atoms pointer. You will have to fix it yourself; above freeptr() will not highlight this error for you.

---

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

It is best to use gdb to determine the exact line with which your program crashes. You do not have to learn gdb commands to do this if you compile your code for debugging.

Without this, replace:

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

from:

 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 */ 

---

All in all, my suggestion is that you are not using C at the moment. Computers are so fast these days that I would question why you read C first.

Do not misunderstand me; I like the language of C. 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 the lack of access to low-level access to computer technology.

In your case, you seem to be a scientist or engineer. I recommend you learn and use Python. Python provides easy-to-read, easily verified programs that you can share with other chemists or engineers. C is not a fast way to write robust code like Python does. In this unlikely future event, which Python is not fast enough for your purposes, there are other solutions that you will then be ready.

Answer 10

Do you have #include <stdio.h> in the file? Interestingly, if you get a call to printf() , which uses an implicit declaration of printf() and, therefore, may use the wrong calling convention.

What do you use for compiler / platform? Do you get any warnings from the assembly?