Why am I having segmentation fault with this code in C?

Question

I am trying to read a line from a file and return it and I want to call this function iteratively as long as I reach EOF or the last line. This is the code.

#include<stdio.h>
#include<string.h>
#include<stdlib.h>

#define BUFF 100000

int grep_stream(FILE* fpntr, char* string, char* filepathname);
char* get_next_line(FILE* fpntr);
int main()
{
    FILE* fp;
    fp = fopen("movies.txt", "r");
    char* filename = "test";
    char* searchString = "the";
    grep_stream(fp, searchString, filename);
    return 0;               
}
int grep_stream(FILE* fpntr, char* string, char* filepathname)
{

    char* tmp = get_next_line(fpntr);
    char* ret;
    while(tmp != NULL)
    {
        ret = strstr(tmp, string);
        printf("%s\n", ret);
        tmp = get_next_line(fpntr);
    }
    return 0;
}

char* get_next_line(FILE* fpntr)
{
    char buff[BUFF];
    int index = 0;
    int ch = fgetc(fpntr);
    while(ch != '\n' && ch != EOF)
    {
        buff[index++] = ch;
        ch = fgetc(fpntr);
    }
    buff[index] = '\0';
    char* tmp;
    /*I am not freeing this tmp, will that be a problem if I call this function iteratively*/
    tmp = (char*)malloc((int)(index)*sizeof(char));
    strcpy(tmp, buff);
    return tmp;
}

And in main, I will be calling the grep_stream() function with valid parameters.

Update: The file that I am trying to add is this: http://textuploader.com/5ntjm

Suggest you use a debugger. At a minimum it will tell you exactly which line is causing the seg fault. If you really need help please post a [minimal complete and verifiable example](https://stackoverflow.com/help/mcve). Also include what the test input is. — kaylum, Mar 15 '16 at 04:02
"I will be calling the grep_stream() function with valid parameters". That is an assertion that really needs to be verified (since by definition you don't actually know where the problem is). Please show the actual code and we will verify for ourselves whether the parameters are valid or not in the context of the entire program. — kaylum, Mar 15 '16 at 04:04
Oh my bad, I am new to programming and hence I do not know how to use a debugger. As for the function call, I have updated the code in my question with extra details. I added the main as well. Thanks. — theprogrammer, Mar 15 '16 at 04:18
As requested please provide the test input file. And I really really suggest you put down what you are doing now and learn at least basic debugger usage. If on Linux a common debugger is [`gdb`](https://www.gnu.org/software/gdb/). It won't be time wasted. It will literally save you hours of debugging and is an essential tool for any dev. — kaylum, Mar 15 '16 at 04:31
Just to prove to you how useful a debugger is, I ran your code with some made up data. It crashed on this line: `printf("%s\n", ret);`. Because `strstr` will return NULL if the string is not found. Hence you need to have `if (ret != NULL) printf("%s\n", ret);`. That literally took me less than 2 minutes to find in a debugger. Convinced yet that you should learn to use a debugger? — kaylum, Mar 15 '16 at 04:40
if you use an IDE, just press run (most probably F5) and it'll stop and the error line. If you use command line just use r in gdb. And [don't cast the result of malloc in C](http://stackoverflow.com/q/605845/995714) — phuclv, Mar 15 '16 at 05:37
in `(int)(index)*sizeof(char)`: `sizeof(char)` is always 1 and no need to specify. The cast to int is redundant too, because it'll automatically be promoted to int anyway — phuclv, Mar 15 '16 at 05:38

score 2 · Accepted Answer · answered Mar 15 '16 at 05:07

ret = strstr(tmp, string);
printf("%s\n", ret);

Is it possible that ret is NuLL here? Perhaps you meant printf("%s\n", ret ? ret : "NO MATCH");

#define BUFF 100000
char buff[BUFF];

That's a huge array. Might it be causing a stack overflow?

char buff[BUFF];
int index = 0;
int ch = fgetc(fpntr);
while(ch != '\n' && ch != EOF)
{
    buff[index++] = ch;
    ch = fgetc(fpntr);
}

If the user enters a line that's too long, this will overflow the buff array. Have you considered placing an upper bound on that loop?

tmp = (char*)malloc((int)(index)*sizeof(char));
strcpy(tmp, buff);

There's way too much casting going on here (there should be none), malloc accepts a size_t (not an int) and sizeof char is always 1. On that note, index should certainly be a size_t.

A string occupies an additional character for the '\0' at the end. You've failed to allocate space for that additional character, so strcpy will overflow your buffer.

Don't forget to check the return value of malloc. You probably meant to write:

tmp = malloc(index + 1);
if (tmp == NULL) {
    return NULL;
}
strcpy(tmp, buff);

Similarly to mallocs return value, you need to check fopens return value:

fp = fopen("movies.txt", "r");
if (fp == NULL) {
    puts("Error opening movies.txt");
    exit(EXIT_FAILURE);
}

On a final note, everything you malloc should be free'd. Your current algorithm leaks a lot of memory. This could cause your program to crash if your file is large enough.

Thanks for your answer. I have tried changing what you have mentioned, but the code is not working still. I still keep getting segfaults for the input file that I am giving, which I uploaded in my question. — theprogrammer, Mar 15 '16 at 08:51

Why am I having segmentation fault with this code in C?

1 Answers1