Segmentation Fault related to the malloc usage in an iterative loop

1 Attachment(s)

Dear Programmers,
I developed a program with the pointer as matm. And now i want to use it iteratively for different files called from the same directory through my program. It is very big program. So i am writing the referred lines here in the attached file (appended below). As soon as the control reaches to second iteration in the last loop at for(i=0;i<1;i++).
It runs correctly only when the i<1 in the above loop. As soon as i> 1, it says Segmentation Failure, doesn't run further and is is aborted.
May you please solve me this issue? I understand somewhere there is some problem with calling the matm pointer. But please guide me where is the problem and find the desired code modules attached referred in the mail.

Please go through the attachment for the same.

Thanks & Regards
Ashish Runthala

The problem has nothing to do with the malloc() call.

The problem is more likely to be because an fopen() call is failing, so your mpdbToArray2() returns a NULL pointer. The main() function opens a file, and then your function immediately attempts to open the same file again. That guarantees the fopen() call in your function will fail.

Since you've apparently edited out code you think is mistaken, I'll bet one of the things you have edited out is that your code is dereferencing said NULL pointer (you need to check a pointer is non-NULL before dereferencing it).

The other thing you need to realise is that C is not like Java (or C#, or any language with garbage collection). Setting a pointer to NULL does not cause any dynamically allocated memory to be released. If you want malloc()'d memory to be released, you should use free() to release it. If you fix the problem of the crash, your program will leak memory every time around the loop.

In forums, it is good practice to provide SMALL but COMPLETE examples of code that exhibit your problem. Taking code that doesn't work, and editing out parts you think are irrelevant is a waste of time. You will invariably leave out something relevant. After all, if you actually knew what was relevant (or irrelevant) to your problem, you would have found the cause without help.

Coordinate File Description (PDB Format)
It might be worth going through your structure and the format description.
I think some of your short char strings are omitting the \0 in calculating how much space to allow.

The other issue I see is that you're opening each file TWICE, and never closing either of them.

1 Attachment(s)

Dear Grumpy,
Now you can check out the code. Your explanation seems good but it is still not correcting the code. Check the attached file and then explain, correct the code.
Or if you give me your mail-ID it would be better. My mail-ID is <<snipped>>

Thanks
Ashish

You missed the point of the last paragraph of my previous post. I was not suggesting you provide the complete code. Instead, you need to work to produce a smaller sample of code that exhibits the same symptom. In the process of producing such a sample of code, you might find the problem on your own. If not, the code you post will be small enough that someone else might be able to help you.

Members of forums like this are not here to do your hard work for you. They are here to help if you have done the hard work to localise your problem, but are still stuck. You have not localised your problem (if you had, you could provide a much smaller sample of code that exhibits the same problem) so you also cannot expect too much help.

1 Attachment(s)

Dear Programmers,
Yes now i understood how to use this forum (Courtesy: Grumpy). Please have a look at the attached file now. and solve the problem in my code. It says Segmentation Failure error.
@Salem: PDB files are correctly being parsed through my code. No error is there for sure.

Thanks
Ashish

I doubt that will compile since MSA_pdbfiles does not appear to have been declared.

MSA_pdbfiles is a different array to extract the available filenames present in the input file (with the extensions) so that their files can also be called iteratively. There is the problem coming up.
After one iteration (i=0), it fails and show segmentation error.
By the way this is the code for MSA_pdbfiles

Code:

//Creating array of PDB filenames to be opened as per the MSA Alignment file char MSA_pdbfiles[MSA_tempcount][20];char MSA_pdb_swap[20]; fseek(MSA_alifile, 0, SEEK_SET); // Seeking the cursor back to start of file i=0; while(fgets(MSA_line,sizeof(MSA_line),MSA_alifile) && (i<MSA_tempcount)) { if((MSA_line[5]=='>')&&(MSA_line[0]!='\0')) { MSA_pdb_swap[0]=MSA_line[0]; //printf("\nSwap0 for %d template=%c",i,MSA_pdb_swap[0]); MSA_pdb_swap[1]=MSA_line[1]; //printf("\nSwap0 for %d template=%c",i,MSA_pdb_swap[1]); MSA_pdb_swap[2]=MSA_line[2]; //printf("\nSwap0 for %d template=%c",i,MSA_pdb_swap[2]); MSA_pdb_swap[3]=MSA_line[3]; //printf("\nSwap0 for %d template=%c",i,MSA_pdb_swap[3]); //strstr(MSA_pdb_swap, "_fit.pdb"); MSA_pdb_swap[4]='_'; MSA_pdb_swap[5]='f'; MSA_pdb_swap[6]='i'; MSA_pdb_swap[7]='t'; MSA_pdb_swap[8]='.'; MSA_pdb_swap[9]='p'; MSA_pdb_swap[10]='d'; MSA_pdb_swap[11]='b'; MSA_pdb_swap[12]='\0'; MSA_line[4] = '\0'; strcpy(MSA_pdbfiles[i],MSA_pdb_swap); printf("\nTemplate %4d:%6s\tStrlen:%d",i,MSA_pdbfiles[i],strlen(MSA_pdbfiles[i])); i++; } }

So see input file available name entries are further converted to their actual filenames to be opened from the current directory.

> @Salem: PDB files are correctly being parsed through my code. No error is there for sure.
And yet it still crashes....

> char line[1000], temp[15][15];
Since you've not actually disputed the link I posted as being your file format, I'm going to assume it is correct.

Two things immediately spring to mind.
1. The record format has 16 fields, whereas you only allow for 15 fields in your temp.

2. You CANNOT use space as a separator.

Code:

1 2 3 4 5 6 7 8 12345678901234567890123456789012345678901234567890123456789012345678901234567890 ATOM 145 N VAL A 25 32.433 16.336 57.540 1.00 11.92 A1 N ATOM 146 CA VAL A 25 31.132 16.439 58.160 1.00 11.85 A1 C ATOM 147 C VAL A 25 30.447 15.105 58.363 1.00 12.34 A1 C ATOM 148 O VAL A 25 29.520 15.059 59.174 1.00 15.65 A1 O ATOM 149 CB AVAL A 25 30.385 17.437 57.230 0.28 13.88 A1 C ATOM 150 CB BVAL A 25 30.166 17.399 57.373 0.72 15.41 A1 C ATOM 151 CG1AVAL A 25 28.870 17.401 57.336 0.28 12.64 A1 C ATOM 152 CG1BVAL A 25 30.805 18.788 57.449 0.72 15.11 A1 C ATOM 153 CG2AVAL A 25 30.835 18.826 57.661 0.28 13.58 A1 C ATOM 154 CG2BVAL A 25 29.909 16.996 55.922 0.72 13.25 A1 C

Some fields do not have a space separating them (and not all fields are present to begin with). This will totally screw up your space dependent tokeniser.

To copy a field, you need something like

Code:

void copyField ( char *to, const char *from, int start, int end ) { int length = end - start + 1; strncpy( to, &from[start-1], length ); to[length] = '\0'; }

So to copy the "ATOM ", you can directly call the function with the values in the spec...
1 - 6 Record name "ATOM "
With
copyField(temp[0], line, 1, 6 );

> int arraylength(Atom * arr)
This is broken as well.
> for(i = 0;strcmp(arr[i].type, "ATOM") == 0;i++)
Since ALL the things you store are ATOM's, then you will inevitably access past the end of your allocated memory in trying to discover an element (which doesn't exist) is an ATOM.

You already have the length calculated in mpdbToArray2
When you call arr = malloc(i*(sizeof(Atom))); , you should consider passing i back as part of the return result (hint: pass a pointer to an int where you want to store the secondary result).

Hey Salem,
But my code is correctly parsing every single entry properly. You tell me how to write for each of the required field. Somewhere it is float, integer or string.
So what you suggested would work solely for strings. So what about others? SECONDLY MY PROGRAM IS WORKING CORRECTLY FOR EVERY SINGLE FILE AND IS NOT GIVING ANY JUNK ANSWER. IT SEEMS TO BE CORRECTLY WORKING AT PDB PARSING AND CALCULATIONS LIKE RMSD, GDT ETC.

arraylength IS ALSO WORKING CORRECTLY. I HAVE USED IT MORE THAN 100 TIMES IN MY PROGRAM WITH ALL RESULTS MANUALLY CHECKED AND ARE CORRECT.

You already have the length calculated in mpdbToArray2
When you call arr = malloc(i*(sizeof(Atom))); , you should consider passing i back as part of the return result (hint: pass a pointer to an int where you want to store the secondary result).
what it means when you say pass a pointer? i am not getting. please elaborate.

Thanks
Ashish

> arraylength IS ALSO WORKING CORRECTLY
You're confusing "working" with "bug-free".
Whilst it may seem that it is "working" to you, rest assured - it is also bug-ridden.

Post one of your data files we can run through your code in post #4 (Added int lm,lt; to make it compile).
I am of course assuming that the code in post #4 will run AND crash. If you've posted some cut-down version which doesn't crash (because you edited away the bug without realising it), then you're just wasting everyone's time. Unless WE can replicate the bug with your code and data, there isn't a lot we can do except issue platitudes.

> what it means when you say pass a pointer? i am not getting. please elaborate.
This is elementary C coding skill - surely you know how to do this:

Code:

void foo ( int *result ) { *result = 42; } int main ( ) { int x; foo(&x); }

If you don't, perhaps you should put down this program and learn some more C.

Oh, and a wall of SHOUTING doesn't make you any clearer to understand.

Quote:

Originally Posted by ashish_runthala

SECONDLY MY PROGRAM IS WORKING CORRECTLY FOR EVERY SINGLE FILE AND IS NOT GIVING ANY JUNK ANSWER. IT SEEMS TO BE CORRECTLY WORKING AT PDB PARSING AND CALCULATIONS LIKE RMSD, GDT ETC.

Your pdb parsing is definitetly wrong. I've put the following line into your program from post #6 at line 94:

Code:

printf("mol: %s\tamino: %s\tchainid: %s\n", arr[k].mol, arr[k].amino, arr[k].chainid);

And here's the output for an example data:

Code:

$ cat test.pdb 1 2 3 4 5 6 7 8 12345678901234567890123456789012345678901234567890123456789012345678901234567890 ATOM 145 N VAL A 25 32.433 16.336 57.540 1.00 11.92 A1 N ATOM 146 CA VAL A 25 31.132 16.439 58.160 1.00 11.85 A1 C ATOM 147 C VAL A 25 30.447 15.105 58.363 1.00 12.34 A1 C ATOM 148 O VAL A 25 29.520 15.059 59.174 1.00 15.65 A1 O ATOM 149 CB AVAL A 25 30.385 17.437 57.230 0.28 13.88 A1 C ATOM 150 CB BVAL A 25 30.166 17.399 57.373 0.72 15.41 A1 C ATOM 151 CG1AVAL A 25 28.870 17.401 57.336 0.28 12.64 A1 C ATOM 152 CG1BVAL A 25 30.805 18.788 57.449 0.72 15.11 A1 C ATOM 153 CG2AVAL A 25 30.835 18.826 57.661 0.28 13.58 A1 C ATOM 154 CG2BVAL A 25 29.909 16.996 55.922 0.72 13.25 A1 C $ ./test mol: N amino: VAL chainid: A mol: CA amino: VAL chainid: A mol: C amino: VAL chainid: A mol: O amino: VAL chainid: A mol: CB amino: AVAL chainid: A mol: CB amino: BVAL chainid: A mol: CG1AVAL amino: A chainid: 25 mol: CG1BVAL amino: A chainid: 25 mol: CG2AVAL amino: A chainid: 25 mol: CG2BVAL amino: A chainid: 25 Seed: test.pdb Hit: test.pdb with length:10

As you can see your parsing for the last 4 lines in the example data doesn't work. And it completely misses field 17 from the pdb specification (alternate location indicator; the "A" and "B" before "VAL" on some lines)

Bye, Andreas

Thanks Andreas, I used the space as column delimiter. May be i need to specify the column numbers for the same. First i should solve this issue and then upload the initial problem. @Salem: When somebody writes the program based on some file, he thinks the simplest way of doing it and others should not think it is trivial. Andreas issue is already correct. But what you have written is entirely the basic code, with no relation to my problem.Should you have the guts, write all possible mistakes as you think and the correcting measures, so that me and andreas can learn it(as you think we are really new to all this). This code has 5673 lines of code. And now i will recheck this mistake. Rest everything is correct, as i checked all the results manually on simple files which had the space separated columns. But this andreas idea would make my code more robust and good for the further development of code.
Don't mind. But write and suggest something which is relevant instead of teaching something very basic. This is what i meant from being more elaborative.

Quote:

Originally Posted by ashish_runthala

Thanks Andreas, I used the space as column delimiter. May be i need to specify the column numbers for the same. First i should solve this issue and then upload the initial problem. @Salem: When somebody writes the program based on some file, he thinks the simplest way of doing it and others should not think it is trivial. Andreas issue is already correct. But what you have written is entirely the basic code, with no relation to my problem.Should you have the guts, write all possible mistakes as you think and the correcting measures, so that me and andreas can learn it(as you think we are really new to all this). This code has 5673 lines of code. And now i will recheck this mistake. Rest everything is correct, as i checked all the results manually on simple files which had the space separated columns. But this andreas idea would make my code more robust and good for the further development of code.
Don't mind. But write and suggest something which is relevant instead of teaching something very basic. This is what i meant from being more elaborative.

You don't listen at all do you?!

Salem has provided extremely good advice, you'd be best to take it all in and act on it.
Andreas simply tried to get you to recognise the points that Salem was making, through repetition and example.
You will not get far as a programmer if you always believe that your code is 100% correct, even when given proof to the contrary.

If you ask a basic question, don't be insulted when given the basic answer. If you think its not relevant then you're failing to match up question and answer, and are probably having language difficulties.

I don't see why I should write your code for you.

You haven't confirmed that the PDB file format we think you're using is the one you're actually using. It's close for sure, but there are a few inconsistencies in what your struct documents as the field numbers and what the reference web page says. There's no point to writing code if the spec is wrong.

Plus, you keep denying that bugs exist (with your "it works for me" refrain), even when they're pointed out to you, and you keep complaining that it crashes. If you were hoping for a "fix this" and sweep the rest of the problems under the carpet, then you're in the wrong place.

The bugs (so far) may not be the actual cause of the current crash, but they will be an issue at some point in the future. So fixing them is a priority for you.

Lesson time:

Code:

#include <stdio.h> #include <stdlib.h> #include <string.h> #include <math.h> #include <ctype.h> #include <errno.h> //Model: Seed Template, Template: Other template hits typedef struct atom { char type[10]; //Type of the structure, i.e. ATOM int an; //Atom Number treated as Index (2nd column) char mol[10]; //Molecule (3rd column) char amino[10]; //Amino Acid (4th column) char chainid[2]; //Chain ID (5th column) int pos; //Position (6th column) float xc; //X Coordinate (7th column) float yc; //Y Coordinate (8th column) float zc; //Z Coordinate (9th column) float occ; //Occupancy float temp; //Temperature Identifier char element[3]; //Element Symbol } atom; void copyField ( char *to, size_t toLen, const char *from, size_t start, size_t end ) { size_t length = end - start + 1; if ( length >= toLen ) length = toLen - 1; // raise an error if this happens strncpy( to, &from[start-1], length ); to[length] = '\0'; } // You could generalise this by passing fields[] as a parameter as well. size_t tokenise ( const char *line, char temp[][15], size_t length ) { static struct { size_t from, to; } fields[] = { { 1, 6 }, // Record name "ATOM " { 7, 11 }, // Integer Atom serial number. { 13, 16 }, // Atom Atom name. { 17, 17 }, // Character Alternate location indicator. { 18, 20 }, // Residue name Residue name. { 22, 22 }, // Character Chain identifier. { 23, 26 }, // Integer Residue sequence number. { 27, 27 }, // AChar Code for insertion of residues. { 31, 38 }, // Real(8.3) Orthogonal coordinates for X in Angstroms. { 39, 46 }, // Real(8.3) Orthogonal coordinates for Y in Angstroms. { 47, 54 }, // Real(8.3) Orthogonal coordinates for Z in Angstroms. { 55, 60 }, // Real(6.2) Occupancy. { 61, 66 }, // Real(6.2) Temperature factor (Default = 0.0). { 73, 76 }, // LString(4) Segment identifier, left-justified. { 77, 78 }, // LString(2) Element symbol, right-justified. { 79, 80 }, // LString(2) Charge on the atom. }; static size_t nFields = sizeof(fields)/sizeof(fields[0]); size_t i; for ( i = 0 ; i < nFields && i < length ; i++ ) { copyField( temp[i], sizeof(temp[i]), line, fields[i].from, fields[i].to ); } // return actual number of tokens extracted return i; } atom *parse ( const char *filename, size_t *numAtoms ) { atom *result = NULL; size_t arrSize = 0; size_t arrLen = 0; FILE *fp = fopen(filename,"r"); if ( fp ) { char buff[BUFSIZ]; char temp[16][15]; size_t tempLen = sizeof(temp)/sizeof(temp[0]); // read each line while ( fgets( buff, BUFSIZ, fp ) != NULL ) { // is it an ATOM, and did it parse OK if ( strncmp(buff,"ATOM ",6) == 0 && tokenise( buff, temp, tempLen ) == tempLen ) { // Extend the array if necessary if ( arrSize == arrLen ) { size_t newSize; // newSize starts at 16, then doubles up to 1MB // then by 1MB if ( arrSize == 0 ) newSize = 16; else if ( arrSize < 1024*1024 ) newSize = arrSize * 2; else newSize = arrSize + 1024*1024; void *t = realloc( result, newSize * sizeof(*result) ); if ( t != NULL ) { result = t; arrSize = newSize; } else { // no more room, but result still valid break; } } // Now copy and convert the fields of interest. // Extra points for validating string lengths prior to strcpy // and for checking the error result of strtol and strtol strcpy(result[arrLen].type,temp[0]); result[arrLen].an = strtol(temp[1],NULL,10); strcpy(result[arrLen].mol,temp[2]); strcpy(result[arrLen].amino,temp[3]); strcpy(result[arrLen].chainid,temp[4]); result[arrLen].pos = strtol(temp[6],NULL,10); result[arrLen].xc = strtof(temp[8],NULL); result[arrLen].yc = strtof(temp[9],NULL); result[arrLen].zc = strtof(temp[10],NULL); result[arrLen].occ = strtof(temp[11],NULL); result[arrLen].temp = strtof(temp[12],NULL); strcpy(result[arrLen].element,temp[14]); arrLen++; } } fclose(fp); } else { perror("Oops"); } *numAtoms = arrLen; return result; } int main ( ) { size_t len, i; atom *arr = NULL; arr = parse( "foo.txt", &len ); printf("Result=%p, len=%zd\n", (void*)arr, len ); for ( i = 0 ; i < len ; i++ ) { printf("%s %d %s %s %s %d %f %f %f %f %f %s\n", arr[i].type, arr[i].an, arr[i].mol, arr[i].amino, arr[i].chainid, arr[i].pos, arr[i].xc, arr[i].yc, arr[i].zc, arr[i].occ, arr[i].temp, arr[i].element ); } free(arr); return 0; }

Code:

$ cat foo.txt 1 2 3 4 5 6 7 8 12345678901234567890123456789012345678901234567890123456789012345678901234567890 ATOM 145 N VAL A 25 32.433 16.336 57.540 1.00 11.92 A1 N ATOM 146 CA VAL A 25 31.132 16.439 58.160 1.00 11.85 A1 C ATOM 147 C VAL A 25 30.447 15.105 58.363 1.00 12.34 A1 C ATOM 148 O VAL A 25 29.520 15.059 59.174 1.00 15.65 A1 O ATOM 149 CB AVAL A 25 30.385 17.437 57.230 0.28 13.88 A1 C ATOM 150 CB BVAL A 25 30.166 17.399 57.373 0.72 15.41 A1 C ATOM 151 CG1AVAL A 25 28.870 17.401 57.336 0.28 12.64 A1 C ATOM 152 CG1BVAL A 25 30.805 18.788 57.449 0.72 15.11 A1 C ATOM 153 CG2AVAL A 25 30.835 18.826 57.661 0.28 13.58 A1 C ATOM 154 CG2BVAL A 25 29.909 16.996 55.922 0.72 13.25 A1 C $ gcc -g -Wall foo.c $ valgrind ./a.out ==3295== Memcheck, a memory error detector ==3295== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al. ==3295== Using Valgrind-3.6.1-Debian and LibVEX; rerun with -h for copyright info ==3295== Command: ./a.out ==3295== Result=0x51d02c0, len=10 ATOM 145 N VAL 25 32.432999 16.336000 57.540001 1.000000 11.920000 N ATOM 146 CA VAL 25 31.132000 16.438999 58.160000 1.000000 11.850000 C ATOM 147 C VAL 25 30.447001 15.105000 58.362999 1.000000 12.340000 C ATOM 148 O VAL 25 29.520000 15.059000 59.174000 1.000000 15.650000 O ATOM 149 CB A VAL 25 30.385000 17.437000 57.230000 0.280000 13.880000 C ATOM 150 CB B VAL 25 30.166000 17.399000 57.373001 0.720000 15.410000 C ATOM 151 CG1 A VAL 25 28.870001 17.400999 57.335999 0.280000 12.640000 C ATOM 152 CG1 B VAL 25 30.805000 18.788000 57.449001 0.720000 15.110000 C ATOM 153 CG2 A VAL 25 30.834999 18.826000 57.660999 0.280000 13.580000 C ATOM 154 CG2 B VAL 25 29.909000 16.996000 55.922001 0.720000 13.250000 C ==3295== ==3295== HEAP SUMMARY: ==3295== in use at exit: 0 bytes in 0 blocks ==3295== total heap usage: 2 allocs, 2 frees, 1,656 bytes allocated ==3295== ==3295== All heap blocks were freed -- no leaks are possible ==3295== ==3295== For counts of detected and suppressed errors, rerun with: -v ==3295== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 4 from 4)

Now, since you mentioned segmentation fault, I'm going to assume you're running on Linux (or a unix of some sort).
In which case, you should be able to get the excellent valgrind utility installed on your development machine.

If you run YOUR code with valgrind (as shown), then I'm betting you're going to see all sorts of memory leaks and buffer overruns.

Example.

Code:

$ cat bar.c #include <stdio.h> #include <string.h> #include <stdlib.h> void foo ( ) { char *p = malloc(10); strcpy(p,"hello world"); } int main(){ foo(); return 0; }

It should be obvious to the average programmer that foo() doesn't allocate enough memory, and doesn't free it when it's done.

Code:

$ gcc -g -Wall bar.c $ valgrind ./a.out ==3312== Memcheck, a memory error detector ==3312== Copyright (C) 2002-2010, and GNU GPL'd, by Julian Seward et al. ==3312== Using Valgrind-3.6.1-Debian and LibVEX; rerun with -h for copyright info ==3312== Command: ./a.out ==3312== ==3312== Invalid write of size 4 ==3312== at 0x40051C: foo (bar.c:7) ==3312== by 0x40052E: main (bar.c:11) ==3312== Address 0x51d0048 is 8 bytes inside a block of size 10 alloc'd ==3312== at 0x4C28F9F: malloc (vg_replace_malloc.c:236) ==3312== by 0x400505: foo (bar.c:6) ==3312== by 0x40052E: main (bar.c:11) ==3312== ==3312== ==3312== HEAP SUMMARY: ==3312== in use at exit: 10 bytes in 1 blocks ==3312== total heap usage: 1 allocs, 0 frees, 10 bytes allocated ==3312== ==3312== LEAK SUMMARY: ==3312== definitely lost: 10 bytes in 1 blocks ==3312== indirectly lost: 0 bytes in 0 blocks ==3312== possibly lost: 0 bytes in 0 blocks ==3312== still reachable: 0 bytes in 0 blocks ==3312== suppressed: 0 bytes in 0 blocks ==3312== Rerun with --leak-check=full to see details of leaked memory ==3312== ==3312== For counts of detected and suppressed errors, rerun with: -v ==3312== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 4 from 4)

The cyan bit is telling you about the illegal memory reference - both where it happened, AND where the memory was allocated. The fix is either to allocate more, or copy less.

The red bit is telling you about the memory leak.