C segmentation fault before/during return statement?

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.

Up vote 3 down vote favorite 1 share g+ share fb share tw.

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

NumBonds; bonds = (int *) malloc(sizeof(int) * numBonds); bonds = atomsnPos. Bonds; for(i = 0; I Type, atomsi. X, atomsi.

Y, atomsi. Z); for(i = 0; I X; diff1 = atomsbondsi - totRead. Y - atomsnPos.

Y; diff2 = atomsbondsi - totRead. Z - atomsnPos. Z; retVal = bondsi - 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((&aminoAcidi), nPos, diff, totRead); printf("%d\n", hPos); c segmentation-fault link|improve this question edited Jun 15 '10 at 20:12Pascal Cuoq17.4k21661 asked Jun 15 '10 at 16:35wolfPack88817 91% accept rate.

– Pete Kirkham Jun 15 '10 at 16:45 7 you've got memory leaks in this code. – Franci Penov Jun 15 '10 at 16:46 I had originally told it to simply exit the code (with an exit(7); statement) if no 'H' was found as that would mean I did something wrong with my data elsewhere, but as I never seemed to run into that problem, I removed that line. – wolfPack88 Jun 15 '10 at 16:47 1 Could you give us the line that causes this segmentation fault?

Run your program in a debugger (gdb for example) and it will give you/us more information about the status of the stack/program at crash time. – Ando Jun 15 '10 at 16:48 1 Small style change you could make: (*amino). NumAtoms can be written as amino->numAtoms.

Makes the code clearer and means the same thing. – Donal Fellows Jun 15 '10 at 20:20.

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.

2 "there's a potential for bug in just about every line of code here" - harsh. True, but harsh. :-) – Franci Penov Jun 15 '10 at 17:48.

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

Thank you for the response either way. – wolfPack88 Jun 15 '10 at 16:45 2 Generally you would be writing past the boundary of some variable on the stack. Strings are notorious for this, but it could be another type of array, or a structure copy or memcpy() type of operation.

– Randy Proctor Jun 15 '10 at 16:49 2 -1 for a generic, hand-wavy answer. – Heath Hunnicutt Jun 15 '10 at 17:40 1 -1 for too generic answer without attempt to actually analyze the code. -1 for wrong guess - from the code, the segfault looks more like accessing already freed heap memory, and not a stack corruption problem.

– Franci Penov Jun 15 '10 at 17:50.

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.

2 I don't think the free() calls are the problem (though they are confusing the issue). The free() call is being made after atom is set to the result of another (useless) malloc(), so it is freeing memory returned from a malloc() call. However, all the calls to malloc() and free() in the function do seem to be pointless.

– Michael Burr Jun 15 '10 at 17:28 I think you're right. I've edited my post to indicate that memory is leaking without claiming that the free calls are the real issue. – Walter Mundt Jun 15 '10 at 17:39.

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

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 bondsi - totRead can be out of the atoms bounds, which could be the segfault.

But the loop won't run again because it returns inside the if... – Spudd86 Jun 17 '10 at 19:48.

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: cs.cmu.edu/~gilpin/tutorial.

You usually can get away with debugging by printing if you remember to end your print with a \n or do a flush() after debug prints. I know it's not the best way to do it, but in some cases it's appropriate. – Shaded Jun 15 '10 at 16:49 2 Printing to stderr works better (use fprintf(stderr,...); rather than printf(...);), since that's really what stderr is for.

A real debugger is better, of course. – David Thornley Jun 15 '10 at 17:00 @David both have uses... if you need to see a long sequence of things extracting that info from a debugger can be painful (extra special painful if it's a gui...) – Spudd86 Jun 17 '10 at 19:50.

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: atomsbondsi - totRead. Type0 == 'H' Make sure you stay inside the array with your index.

Well, the pointer is currently pointing to data that is being used in another array. I don't want to have an extra pointer pointing to the same data, so I have it point to something else, and then free it to avoid memory leaks. As I am a relatively new programmer, I'm not sure this is good practice, but it seemed like a good idea at the time.

– wolfPack88 Jun 15 '10 at 16:57 Lucas has a correct point. It can (almost*) never be correct to perform a malloc(), and free the same memory in the immediately following statement. From reading your code it is apparent that you should be free()-ing your memory before reusing the pointer for newly allocated memory.

– Heath Hunnicutt Jun 15 '10 at 17:00 1 @wolfPack88: I am not sure I understand you. If you don't want your pointer pointing into some other memory you should point it to NULL, but I don't think that is what you want here. – Lucas Jun 15 '10 at 17:04 1 @wolfPack88: You actually don't need to set atoms or bonds to anything when you're done using them - they're local variables, just stop using them when you no longer want to reference the arrays that belong to another structure.

Get rid of all the calls to malloc() and free() in the function - they don't do anything useful and cause confusion (as well as being the source of some memory leaks that have nothing to do with the segfault). – Michael Burr Jun 15 '10 at 17:24.

EDIT: go read this stackoverflow.com/questions/233148/c-poi... 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 = atomsnPos. NumBonds; bonds = (int *) malloc(sizeof(int) * numBonds); bonds = atomsnPos. 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 = atomsnPos.

NumBonds; bonds = atomsnPos. Bonds; for(i = 0; I numAtoms; i++) printf("ATOM\t\t%d %s\t0001\t%f\t%f\t%f\n", I + 1, atomsi. Type, atomsi.

X, atomsi. Y, atomsi. Z); for(i = 0; I Type0 == 'H') { diff0 = atomsbondsi - totRead.

X - atomsnPos. X; diff1 = atomsbondsi - totRead. Y - atomsnPos.

Y; diff2 = atomsbondsi - totRead. Z - atomsnPos. Z; retVal = bondsi - totRead; printf("2 %d\n", retVal); return retVal; } }.

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?

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

The problem is they then write to the memory pointed to by the same pointer they just freed. – Heath Hunnicutt Jun 15 '10 at 17:41 1 nope, they return right after freeing so no dangling references (it goes like this atoms = malloc; atoms = amino->atoms; atoms=malloc; free(atoms); so they end up leaking sizeof(Atom)*numAtoms bytes but don't free anything that should not be free'd – Spudd86 Jun 15 '10 at 18:22.

I cant really gove you an answer,but what I can give you is a way to a solution, that is you have to find the anglde that you relate to or peaks your interest. A good paper is one that people get drawn into because it reaches them ln some way.As for me WW11 to me, I think of the holocaust and the effect it had on the survivors, their families and those who stood by and did nothing until it was too late.