Segmentation fault : Reading values from a text file

Question

In my code I am trying to read values from a .txt file so as to build my adjacency matrix but it keeps on returning a segmentation fault. I don't seem to be able to point out where I am going wrong. Please help.

#include <stdio.h>
#include <unistd.h>
#include <ctype.h>
#include <string.h>
#include <stdlib.h>
#include <time.h>
#include <sys/time.h>
#include <math.h>
#include <limits.h>
#include <iostream>


#define MAX_VERTICES 1024

int global_adj_matrix[MAX_VERTICES][MAX_VERTICES];


int **graph_tree;
int **node_data;
int global_weight;
int number_threads;
int max_nodes;
int random_node;
int max_weight;
int finish_flag;

void readAdjMatrix();


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

    for(int i = 0 ; i < MAX_VERTICES ; i++){
        for(int j = 0 ; j < MAX_VERTICES ; j++){
            global_adj_matrix[i][j] = 0;
        }
    }

    number_threads = atoi(argv[1]);
    max_nodes = 0;

    readAdjMatrix();

}

void readAdjMatrix(){

    int source, destination, edge_weight;

    max_nodes = INT_MIN;
    max_weight = INT_MIN;

    FILE *file_pointer = fopen("graph.txt", "r");

    while(!feof(file_pointer)){
        fscanf(file_pointer, "%d", &source);
        fscanf(file_pointer, "%d", &destination);
        fscanf(file_pointer, "%d", &edge_weight);

        global_adj_matrix[source][destination] = edge_weight;
        global_adj_matrix[destination][source] = edge_weight;

        if(edge_weight > max_weight)
            max_weight = edge_weight;

        if(destination > max_nodes)
            max_nodes = destination;
    }

    printf("%d %d", max_weight, max_nodes);

    for(int i = 0 ; i <= max_nodes ; i++){
        for(int j = 0 ; j <= max_nodes ; j++){
            printf("%d\t", global_adj_matrix[i][j]);
        }
        printf("\n");
    }

    fclose(file_pointer);
}

This is my .txt file

If it is actually C, remove the `` include, because that is C++. You are not using any element of iostream, so you do not need it. — harmic, Apr 24 '17 at 04:29
It only segfaults for me if I do not supply an argument. How are you executing it - do you supply any arguments? — harmic, Apr 24 '17 at 04:30
From core are you able to identify where it fails? Also you need to add check if fopen returns null — Pras, Apr 24 '17 at 04:37
It fails when I do not enter an argument. Actually I was working on an OpenMP assignment and I am kind of a noob. Thanks a ton. — Abhijeet Mohanty, Apr 24 '17 at 04:40
If you don't enter an arg then it should be obvious that `atoi(argv[1]);` will cause a problem because `argv[1]` will be NULL. Suggest you learn to use a debugger. — kaylum, Apr 24 '17 at 04:45
This is not C, C doesn't have iostream`. Don't use inappropriate language tags. Select a language and write in it. — n. m. could be an AI, Apr 24 '17 at 04:56
I am perfectly able to execute your code without getting any segmentation fault !! Why is it that you are not able to execute this ? — Coding Ninja, Apr 24 '17 at 07:00
No doubt you gave a very good try .... but, can you please explain why you are taking an input via the command line when you are using it no where ? — Coding Ninja, Apr 24 '17 at 07:04
@AbhijeetMohanty, your code works fine even though you have that ! I have executed it successfully on my machine without any segmentation fault. — Coding Ninja, Apr 24 '17 at 07:33
Why have you have neglected this bigger code ? What is this bigger code doing with the smaller one ? How am I suppose to deliver the solution by keeping this neglected bigger code in mind ? Please let me know if you want me to neglect it like you have did. I post the answer as a whole for you. — Coding Ninja, Apr 24 '17 at 07:40
I was hitting a segmentation fault in the main() itself and I want to sort things out on my own first and only then ask it as a doubt as SO responses are quick and correct, so just wanted to do it on my own. Thanks a ton. @CodingNinja — Abhijeet Mohanty, Apr 24 '17 at 07:44
It is still not clear whether you want me to post the answer by neglecting the larger code or not. Do you want me to neglect the bigger code and post the answer ? — Coding Ninja, Apr 24 '17 at 07:50
Yes an explanation would be nice but as far as debugging is concerned I had forgotten to mention the argument at command line. — Abhijeet Mohanty, Apr 24 '17 at 07:52

Micrified · Answer 1 · 2017-04-24T16:56:15.490

Your segmentation fault is because you're trying to read a nonexistent index in the argument vector of main. If you want to avoid that, you should rewrite it to match something like this:

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

    if (argc > 1 && (number_threads = atoi(argv[1]))) {
        max_nodes = 0;
        readAdjMatrix();
    }

    return 0;
}

This ensures that you have an argument to convert to begin with, and also that it is a nonzero number. I believe atoi has undefined behavior if it isn't a valid string though, so you should harden against that. You also do some other unnecessary things. For one, this block here:

   for(int i = 0 ; i < MAX_VERTICES ; i++){
        for(int j = 0 ; j < MAX_VERTICES ; j++){
            global_adj_matrix[i][j] = 0;
        }
    }

is pointless because if you initialize a 2D array as an external/global variable then it is automatically zeroed upon initialization. Only local/automatic variables will be filled with garbage data. Therefore, you can omit it.

Finally, I would also change your while loop to look more or less like this (Credit: Chux for better loop guard).

while(fscanf(file_pointer, "%d %d %d", &source, &destination, &edge_weight) == 3) {

    global_adj_matrix[source][destination] = global_adj_matrix[destination][source] = edge_weight;

    if(edge_weight > max_weight)
        max_weight = edge_weight;

    if(destination > max_nodes)
        max_nodes = destination;
}

This ensures you correctly scanned the amount of variables necessary per line. And the extended assignment just saves a bit of room.

Hope this fixed the problem you were having.

Will put your version in instead. Thank you! – Micrified Apr 24 '17 at 16:54 — Micrified, Apr 24 '17 at 16:54

Coding Ninja · Answer 2 · 2017-04-26T13:32:49.387

0

I am posting my answer by neglecting the bigger code which you have not mentioned in your question. I have trimmed down the unnecessary code. The code is given below.

 #include <stdio.h>
 #include <unistd.h>
 #include <ctype.h>
 #include <string.h>
 #include <stdlib.h>
 #include <math.h>
 #include <limits.h>

 #define MAX_VERTICES 1024

 int global_adj_matrix[MAX_VERTICES][MAX_VERTICES];
 int global_weight,max_nodes,random_node,max_weight;

 void readAdjMatrix();


 int main()
 {
  int i,j;

   max_nodes = 0;
   readAdjMatrix();
   return 0;
 }


 void readAdjMatrix()
 {
  int source, destination, edge_weight,i,j;
  max_nodes = INT_MIN;
  max_weight = INT_MIN;

  FILE *file_pointer = fopen("graph.txt", "r");

  while(!feof(file_pointer))
  {
    fscanf(file_pointer, "%d", &source);
    fscanf(file_pointer, "%d", &destination);
    fscanf(file_pointer, "%d", &edge_weight);

    global_adj_matrix[source][destination] = global_adj_matrix[destination][source] =edge_weight;


    if(destination > max_nodes)
        max_nodes = destination;
   }

   printf( "%d\n", max_nodes);

   for( i = 0 ; i <= max_nodes ; i++){
    for( j = 0 ; j <= max_nodes ; j++){
        printf("%d\t", global_adj_matrix[i][j]);
    }
    printf("\n");
   }

   fclose(file_pointer);
  }

PS : Simply execute this code with ./a.out with no command line argument. In case you are using the command line argument (as given in your question), please use the following syntax to execute your code :

./a.out "your desired number which works with the bigger code"

edited Apr 26 '17 at 13:32

answered Apr 24 '17 at 08:24

Coding Ninja

114
12

`while(!feof(file_pointer))` test is too late: code will write outside `global_adj_matrix[][]`. Better to test the result of `fscanf()`s. – chux - Reinstate Monica Apr 24 '17 at 13:40
How come ? The two indexes of the `global_adj_matrix[][]` array are being taken from the file (column 1 --> first index, column 2 --> second index and column 3 --> value to be stored at that location). None of the values in the file are exceeding the number `1024`. How will the data be written outside the matrix ? – Coding Ninja Apr 25 '17 at 07:49
After the last valid line is read with `fscanf(file_pointer, "%d", &edge_weight);`, the `while(!feof(file_pointer))` does _not_ prevent the next iteration as end-of-file has not happened yet. The 3 `fscanf()` read fail to read. In this case, likely the values of `source, destination` remain unchanged. Yet code is brittle. Should `int source, destination` get moved to inside the `while(!feof(file_pointer)) {` loop, their value would be undefined. Best to check the return of `fscanf()` & ditch `feof()`. See http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong – chux - Reinstate Monica Apr 25 '17 at 14:18
I really don't understand how you are telling this --> " the while(!feof(file_pointer)) does not prevent the next iteration as end-of-file has not happened yet". Of course, as stated in this link [http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong] , it is best to use `(ch = fgetc(fp)) != EOF)` instead of `while(!feof(fp))`, but it is definitely the end of file as stated in the question. `fscanf` will scan string by string and will terminate correctly. I have executed the code perfectly (along with the text file). – Coding Ninja Apr 26 '17 at 05:41
After `fscanf(file_pointer, "%d", &edge_weight);` do `printf("%d\n", edge_weight);` and report if the _last_ `edge_weight` is printed once or twice. – chux - Reinstate Monica Apr 26 '17 at 12:04
Yes ! The last value is being printed twice ! I am not able to get you ....My question still hangs in balance ---> Why do you say that the values will be printed outside the array ? Please explain in detail. – Coding Ninja Apr 26 '17 at 13:40
1

On that last `fscanf(file_pointer, "%d", &source);`, `fscanf()` did not return 1 - the scan failed. Try `fscanf(file_pointer, "%d", &source);` --> `printf("retval:%d\n", fscanf(file_pointer, "%d", &source));`. Since it do not return 1, the value of `source` will be 1) unassigned or 2) potentially corrupted. If corrupted or never assigned, `global_adj_matrix[source][destination] = ...` is undefined behavior - access outside the array may be attempted. Robust code checks the return values of `*scanf()`. – chux - Reinstate Monica Apr 26 '17 at 13:52
1

Rather than `while(!feof(file_pointer)) { fscanf(file_pointer, "%d", &source); fscanf(file_pointer, "%d", &destination); fscanf(file_pointer, "%d", &edge_weight);` use `while(fscanf(file_pointer, "%d%d%d", &source, &destination, &edge_weight) == 3) {` – chux - Reinstate Monica Apr 26 '17 at 13:53

Segmentation fault : Reading values from a text file

2 Answers2