Trouble with fgets() and strcpy()

Question

I'm writing a C program which begins by opening and reading a file of 50 movie titles, each written on a single line. Next I am attempting to assign each line (or movie title) of the file into each element of an array called FilmArray[51]. I am using strcpy() to do so but the program crashes each time it reaches the first loop of the strcpy and I cannot seem to figure out where I've gone wrong...

int main()
{
int i=0;
char array[51];
char FilmArray[51];
bool answer;

FILE *films;
films = fopen("filmtitles.txt", "r");

if(films == NULL){
    printf("\n ************* ERROR *************\n");
    printf("\n \"filmtitles.txt\" cannot be opened.\n");
    printf("\n         PROGRAM TERMINATED\n");
    exit(EXIT_FAILURE);
}

while(fgets(array, sizeof array, films) != NULL){
    printf("%d. %s",i,  array);
    strcpy(FilmArray[i], array);
    i++;
}

Are you using a debugger? Where does it crash? Do you get a runtime error? — Dai, Mar 04 '15 at 00:11
`char FilmArray[51];... strcpy(FilmArray[i], array);` should generate a compiler warning. Save time. enable all your compiler warnings. — chux - Reinstate Monica, Mar 04 '15 at 00:15
Another good idea would be to distinguish between the length of the title and the number of titles . You confused yourself by using 50 for both — M.M, Mar 04 '15 at 00:21
as well as the other fixes suggested, `#include ` and `#include ` are required. If you're using a very old compiler then applying the proper includes increases the chance you'll get a warning for mistakes like this. — M.M, Mar 04 '15 at 00:27
the posted code is incomplete/not compilable. on SO, posted code is expected to compile and to demonstrate the problem. — user3629249, Mar 04 '15 at 00:34

score 4 · Answer 1 · answered Mar 04 '15 at 00:15

4

FilmArray is an array of characters, not an array of strings.

So when you're doing

strcpy(FilmArray[i], array);

the compiler will convert the value in FilmArray[i] to a pointer and use that as the destination of the string. This will lead to undefined behavior.

In fact, the compiler should be shouting a warning at you for this, and if it doesn't then you need to enable more warnings because warnings are messages about things that the compiler think are suspect and may lead to UB.

answered Mar 04 '15 at 00:15

Some programmer dude

400,186
35
402
621

1

Further description: in C99 this is ill-formed ; converting that value to a pointer is a possible course of action (but not a very sensible one, I'm not clear on why compiler vendors think it's a good idea). In C89 it's UB with no diagnostic required (and no conversion). – M.M Mar 04 '15 at 00:25

score 0 · Answer 2 · answered Mar 04 '15 at 00:22

0

Your code is essentially trying to do an integer to pointer conversion passing char to parameter of type char *. Try using & with strcpy :

strcpy(&FilmArray[i], array);

& designates the use of a pointer for FilmArray[i]

answered Mar 04 '15 at 00:22

l'L'l

44,951
10
95
146

score 0 · Answer 3 · answered Mar 04 '15 at 00:49

suggest the following code
which compiles with no errors/warnings
is complete (given the posted code)
eliminates a lot of the code clutter
eliminates the 'magic' numbers buried in the code

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

#define MAX_NUM_FILMS (51)
#define MAX_FILM_TITLE_LEN (51)

int main()
{
    int  i=0;
    char FilmArray[ MAX_NUM_FILMS ][ MAX_FILM_TITLE_LEN ] = {{'\0'}};

    FILE *films;
    films = fopen("filmtitles.txt", "r");

    if(films == NULL)
    {
        printf("\n ************* ERROR *************\n");
        printf("\n \"filmtitles.txt\" cannot be opened.\n");
        printf("\n         PROGRAM TERMINATED\n");
        exit(EXIT_FAILURE);
    }

    // implied else, fopen successful

    while(fgets(&FilmArray[i][0], MAX_FILM_TITLE_LEN, films))
    {
        printf("%d. %s\n",i,  FilmArray[i]);
        i++;
    }

    // other code here

    fclose( films ); // cleanup
    return(0);
} // end function: main

It runs off the end of `FilmArray` if you read too many (buffer overflow). While this is most probably just a coding exercise, I'd encourage actually using a maximum value if it's defined to avoid such problematic behaviors. `while (i < MAX_NUM_FILMS && fgets(&FilmArray[i][0], MAX_FILM_TITLE_LEN, films))` for example. — , Mar 04 '15 at 01:00

Trouble with fgets() and strcpy()

3 Answers3