Structures and Pointers in C - crash when using strcpy

Question

I have an assignment that is supposed to be written in C (not C++), in which I need to create some structs from reading multiple text files. I have learnt c before (2 years ago) - I'm far more comfortable with Java, just can't use that for this project. I guess my issue comes from not understanding the pointer syntax very well :/. However, my real issue:

The code I have written crashes when I try to use the strcpy function:

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

typedef struct{
    char* filename;
    int time;
} JOB;

JOB **jobQueue;
int nJobs;

void trimLine(char* line) {
    for (int i = strlen(line); i >=0; i--) {
        if (line[i] == '\n' || line[i] == '\r') line[i] = '\0';
    }
}

int main(int argc, char* argv[]) {
    if (argc !=2) {
        printf("Error - Usage is: my_project file\n");
        exit(-1);
    }
    FILE *fp;
    fp = fopen(argv[1],"r");
    if (fp==NULL) {
        printf("Error - file %s could not be read.\n",argv[1]);
        exit(-1);
    }
    jobQueue = malloc(3*sizeof(JOB*));
    char filename[BUFSIZ];
    nJobs = 0;
    while (fgets(filename,sizeof(jobfilename),fp)!=NULL) {
        trimLine(filename);    
        JOB* newjob;
        newjob = malloc(sizeof(JOB));
            //** THIS IS WHERE IT SCREWS UP
        strcpy(newjob->filename,filename);

        jobQueue[nJobs++] = newjob;
    }
}

If I delete the line containing strcpy, the program runs fine (well, I realise this part doesn't really do anything, but still). However, when the program contains the strcpy line, it breaks when attempting to do Job #2. Any idea why?

Also: If I need to maintain an array of JOBs for use in other functions, is the way I have done it correct? JOB **jobQueue is an array of pointers to JOBs, JOB *newjob is a pointer to a JOB, would this work correctly?

Answer 1

newjob->filename is a wild pointer(not set to anything), you have to allocate memory before you can store things at it.

Answer 2

Change:

typedef struct{
    char* filename;
    int time;
} JOB;

to:

#include <limits.h>

typedef struct{
    char filename[PATH_MAX];
    int time;
} JOB;

Answer 3

Additional suggestions for improvement of your code:

• You do never free() the malloced pointers.

• What is there are more than 3 Jobs? Your code doesn't handle this. You could use a linked list instead of an array.

• You do not call fclose() on your file handle.

Answer 4

I'd like to add a few suggestions

nJobs = 0;

Globals are initialised with 0, you don't need to do it manually.

while (fgets(filename,sizeof(jobfilename),fp)!=NULL) {

jobfilename is not declared in your code. I guess you mean filename.

for (int i = strlen(line); i >=0; i--) {
    if (line[i] == '\n' || line[i] == '\r') line[i] = '\0';
}

You start with the ending \0 which you could skip.

You declare new variables everywhere you like, it's good practice (and C89 standard) that increases readability to declare variables at the start of a code block.

Answer 5

Trasvi, don't forget that your jobQueue is malloc'ed to hold only 3 instances of the JOB struct. However, your "while loop" goes around as many times as the user inputs.

But to answer your original question, simply add this to your code before the strcpy.

newjob->filename = malloc ( strlen( filename) +1 );
//You only need to malloc the amount of characters in the filename + 1,
//which is for the null-terminated char, and you don't need to worry about 
//multiplying by 'sizeof' because a char is one byte on any compiler.