Seg fault on last line of code

Question

I'm hoping to implement a simple molecular dynamics program. My first step is to define the system as a series of atoms, each with a type, an id number, a 3 dimensional position vector, and a 3D velocity vector. Below is the program I've written to do so:

FILE *init;

static int randomVelocity(void)  
{  
     return rand()/RAND_MAX - 0.5;  
}  


int main(int argc, char *argv[])  
{  

    int iType;  
    int iID;  
    int i;  
    double* pdPosition;  
    double* pdVelocity;  
    char* line;  
    Atom* poAtoms;  
    int count = 0;  

    init = fopen("newdat.txt", "r+");  
    srand((unsigned)time(NULL));  
    line = malloc(81*sizeof(char));  
    while (fgets(line, 80, init) != NULL)    
    {  
         char* tok1;  
         char* tok2;  
         char* tok3;  
         char* tok4;  
         tok1 = strtok(line, " \t");  
         if ((tok1 == NULL) || (tok1[0] == '*'))   
         {  
              break;  
         }  
         tok2 = strtok(NULL, " \t");  
         tok3 = strtok(NULL, " \t");  
         tok4 = strtok(NULL, " \t");  
         iType = atoi(tok1);  
         iID = count;  
         pdPosition = (double*)malloc(3*sizeof(double));  
         pdVelocity = (double*)malloc(3*sizeof(double));     
         pdPosition[0] = atof(tok2);  
         pdPosition[1] = atof(tok3);  
         pdPosition[2] = atof(tok4);  
         pdVelocity[0] = randomVelocity();  
         pdVelocity[1] = randomVelocity();    
         pdVelocity[2] = randomVelocity();  
         poAtoms[count] = Atom_new(iType, iID, pdPosition, pdVelocity);  
         count++;  
    }  

    for (i = 0; i < count; i++)  
    {  
         Atom_print(poAtoms[i]);  
         Atom_free(poAtoms[i]);  
    }   

    free(line);  
    return 0;  
}

Here is the header file atom.h:

/**** atom.h ****/


typedef struct Atom_str *Atom;  

Atom Atom_new(int iType, int iID, double* adPosition, double* adVelocity);  

void Atom_free(Atom oAtom);  

void Atom_print(Atom oAtom);

and the test input file:

1 5 7 9  
2 12 13 14

The program compiles, but when I run it, I get the expected output followed by a seg fault. I'm using the GDB debugger, and the seg fault appears to happen on the very last line of code, after the return statement! Is it a memory management issue?

These types of problems are often related to memory corruption. Can you type "bt" after the crash in GDB and see if there's a stack trace, and post it if so? Or use Valgrind. — Dan Fego, Jan 01 '12 at 20:07
Is this the stack trace you were referring to? #0 0x0804b3fd in ?? () #1 0x00000001 in ?? () #2 0xffffd8b4 in ?? () #3 0xffffd8bc in ?? () #4 0x00722828 in ?? () #5 0x00000000 in ?? () — arafasse, Jan 01 '12 at 21:06

Daniel Fischer · Accepted Answer · 2012-01-01T21:59:33.333

6

You've never malloced memory for poAtoms. Writing to wherever that uninitialized pointer points can easily cause a segfault.

Before you start reading the file, you should allocate some space,

unsigned expected_count = 2; // for the test input file, would be much larger in real runs
poAtoms = malloc(expected_count*sizeof(*poAtoms));

And then you have to check inside the read-loop that you're not writing past the allocated memory. Before

    poAtoms[count] = Atom_new(iType, iID, pdPosition, pdVelocity);

insert a check,

    if (expected_count <= count)
    {
        expected_count *= 2;   // double the space, could also be a smaller growth factor
        Atom *temp = realloc(poAtoms, expected_count*sizeof(*poAtoms));
        if (temp == NULL)
        {
            perror("Reallocation failed, exiting\n");
            exit(EXIT_FAILURE);
        }
        poAtoms = temp;
    }

if the allocated space for poAtoms is already used, try to get more with realloc, if that fails, abort unless you know how to fix it. If the reallocation succeeds, we can continue collecting new atoms.

edited Jan 01 '12 at 21:59

answered Jan 01 '12 at 20:11

Daniel Fischer

181,706
17
308
431

Thanks for your reply, Daniel! I am providing the code for my atom.c file: struct Atom_str { int iType; int iID; double* pdPosition; double* pdVelocity; }; Atom Atom_new(int iType, int iID, double* adPosition, double* adVelocity) { Atom oAtom; oAtom = (Atom)malloc(sizeof(struct Atom_str)); if (oAtom == NULL) return NULL; oAtom->iType = iType; oAtom->iID = iID; oAtom->pdPosition = adPosition; oAtom->pdVelocity = adVelocity; return oAtom; } – arafasse Jan 01 '12 at 20:51
Sorry for the strange formatting there... but what I meant to say was that I did malloc space for the atom struct using the line "oAtom = (Atom)malloc(sizeof(struct Atom_str));" Am I not malloc-ing enough memory, perhaps? – arafasse Jan 01 '12 at 20:54
Well, that's irrelevant here, because between the declaration `Atom* poAtoms;` and the write `poAtoms[count] = Atom_new(iType, iID, pdPosition, pdVelocity);`, there is no mention whatsoever of `poAtoms`. So `poAtoms` points to whatever the bits that accidentally occupied its position on the stack indicate, and that's where the return value of `Atom_new` is written. In your case, that seems to have overwritten part of the program exit code. – Daniel Fischer Jan 01 '12 at 21:03
@user1125353 you're `malloc`ing for the `struct Atom_str`, the payload, but not for the `Atom` pointers to the payload. – Daniel Fischer Jan 01 '12 at 21:04
Okay, I think I see... Are you saying that poAtoms is actually an array of pointers? In which case, should I declare it as "Atom** poAtoms;"? And then should I include a line like "poAtoms[count] = (Atom*)malloc(sizeof(int));"? (Because a pointer is just an int address, correct?) – arafasse Jan 01 '12 at 21:37
Since an `Atom` is a pointer to `Atom_str`, `poAtoms` is indeed an array of pointers to `Atom_str` (by intent, not exactly since arrays aren't pointers). The declaration of `poAtoms` is correct, as `Atom*` it is an `Atom_str**`. I'll put a correct malloc line in the answer for readability. Note: a pointer is not an int address, for one, it's unsigned (doesn't matter for the size), more importantly it's often the size of a `long`, on 64-bit systems a pointer is 8 bytes, while int is typically 4. – Daniel Fischer Jan 01 '12 at 21:46
Ah, that makes sense; thank you so much! I'm sorry though - where did you put the correct malloc line? (I'm new to this forum and don't know my way around just yet...) – arafasse Jan 01 '12 at 21:58

score 0 · Answer 2 · answered Jan 01 '12 at 20:22

0

What is tok1[0] doing in if condition ? Did you check a small code snippet for strtok that what is returned or checked when you try to print tok1[[0]?

answered Jan 01 '12 at 20:22

Invictus

4,028
10
50
80

score 0 · Answer 3 · answered Jan 01 '12 at 20:39

0

Can you explain what does 1 5 7 9 represent in input file ? are thei type id and velocity . If so you can manipulate and differentiate them by hyphen and extract each value by strtok as NULL. Also check if fgets is appending NULL or may be * as you have tried to check in if condition probably for End of line . I have a gut felling that it can be the place where you are going wrong , Or if you can pleas explain your input file it would be helpful

answered Jan 01 '12 at 20:39

Invictus

4,028
10
50
80

Thanks for your reply! 1 is the atom type; the 5, 7, and 9 should be the x, y, and z components of the position vector, respectively. (The three components of the velocity vector are provided by a random number generator.) The if (tok1[0] == '*') part of my code is supposed to let my input file have comments, delimited by '*'... but I probably need to think that through a bit more! :) – arafasse Jan 01 '12 at 20:49

Seg fault on last line of code

3 Answers3