1

I'm having the weirdest problem ever when programming in C. My function sometimes runs and other times it doesn't, and even though I tried searching for these errors (stack smashing detected and no source for getenv) I can't seem to find the answer to why it fails sometimes. I tried debugging it and it only has a problem in the last character (a "}"), so it runs all those other functions (and they have been tested a lot separately) but sometimes it just falls apart and doesn't run the last function (it works, that I can guarantee, because some other times it even runs inside this function). Also the few last times I ran the function it gave Segmentation Fault even though sometimes it ran all the way to the end. Is there any way I can debug this problem? Here goes my code:

void main(int argc, char * argv[]) {
FILE * fin=fopen(argv[1],"r");
char v[1024];
int col;
matrix m=emptyS();
while(fscanf(fin, "%s",v)!=EOF) {
    int i=0;
    int * w = (int*) malloc(sizeof (int));
    int str=strlen(v);
    int size=0;
    while(i<str) {
        char a[4];
        int y;
        for(y=0;y<4;y++)
            a[y]='\0';
        int x=0;
        while(v[i]!=',') {
            a[x]=v[i];
            i++;
            x++;
        }
        i++;
        size++;
        w=realloc(w,size*sizeof(int));
        w[size-1]=atoi(a);
    }
    m=add(m,w,size);
    col=size;
}
fclose(fin);
graphW wg=discWD(col-1);
int k=0;
while(k<(col-2)) {
    int j=k+1;
    while(j<(col-1)) {
        wg=add_Wedge(wg,k,j,mi(m,k,j,col));
        j++;
    }
    k++;
}
        int* ms=MST(wg);
graph gbayes=tree(wg,ms);
bayes b=newBN(gbayes,m);
FILE * fout=fopen(argv[2],"w");
serialize(b,fout);
fclose(fout);
}

Thank you in advance!

PL-RL
  • 427
  • 2
  • 7
  • 13
  • 1
    Did you compile your code with all warnings enabled and debugging info (e.g. `gcc -Wall -g` on Linux)? Did you use the debugger (e.g. `gdb`) ? – Basile Starynkevitch May 26 '12 at 14:18
  • "My function sometimes runs and other times it doesn't" - it would be more helpful to state which function (i.e. by name), and do you mean that the function is not being called when it should be, or that an error occurs when it is called? – gcbenison May 26 '12 at 14:20
  • 1
    "Is there any way I can debug this problem?" Using a debugger? – Niklas B. May 26 '12 at 14:21
  • I'd suggest you rewrite this as a set of smaller functions and then test each individually. You have three levels of nested loops which is a bad sign. Also remove the cast from malloc (ie `int *w = malloc(sizeof (int))` – William Morris May 26 '12 at 14:24
  • I don't think that these randomly placed declarations lead to Good Style. Most of the while() loops could be condensed into *more readable* for() loops, without having these i++, j++ scattered all over the place. – wildplasser May 26 '12 at 14:35
  • @gcbenison this main function is the one that sometimes finishes running and other times it doesn't. NiklasB. Obviously I have tried that, it just cannot show me the problem... WilliamMorris I tested all functions separately, they work just fine, this function is the one having problems – PL-RL May 26 '12 at 14:39
  • the `int * w = (int*) malloc(sizeof (int));` line leaks memory. – wildplasser May 26 '12 at 14:49
  • @wildplasser thank you, so I should just free this array after adding it to my matrix? – PL-RL May 26 '12 at 14:53
  • No, you could move the declarations to the top of the function. The code will look less messy, and you'll spot real errors easier. The variables could have more descriptive names, too. – wildplasser May 26 '12 at 14:58
  • @wildplasser thanks, but that doesn't solve the problem of memory leakage, I will move my declarations to the top of the function, but isn't it correct to free this array as I was saying? – PL-RL May 26 '12 at 15:06
  • I could not deduce the goal of the program from the source. It looked like it does some parsing / summarising / aggregating, and the final result is in TheBigArray (with a very short name). In that case, you would want to keep the array at the end of all the loops. – wildplasser May 26 '12 at 15:10
  • @wildplasser I think there is no problem, because my add function (when adding the array to the matrix) copies my array to the heap, so I only have it declared with a malloc because I don't know it's size at first, is there any way it can become simpler? – PL-RL May 26 '12 at 15:15
  • A trivial first step would be to put all the read_file_into_int_array stuff into a function. The array could be the return value, but since you also need to communicate the number of elements it could also be a struct with the array and the number of elements in it. Also: main() shall return int. – wildplasser May 26 '12 at 15:36

1 Answers1

3

You don't check x for going out of bounds. Valgrind would have told you.

while(i<str) {
        char a[4] = { 0 };
        int x=0;
        while(v[i]!=',') {
            a[x]=v[i]; /* here you access a[x] without check for x */
            i++;
            x++; /* Here it may go >= 4*/
        }
        i++;
        size++;
        w=realloc(w,size*sizeof(int));
        w[size-1]=atoi(a);
    }
Jens Gustedt
  • 76,821
  • 6
  • 102
  • 177
  • Thank you very much! I think you just solved my problem! Only one additional question, knowing I'm re-declarating x every time I go in this while loop, shouldn't it work just fine? – PL-RL May 26 '12 at 14:43