CODEWITHSUNDEEP
c
Trasvi
Trasvi

Reputation: 1247

Structures and Pointers in C - crash when using strcpy

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?

Upvotes: 0

Views: 3567

Answers (6)

Victor Carrera
Victor Carrera

Reputation: 101

You have a null pointer in newjob->filename:

int nJobsMax=3;
char* filename;
JOB* newjob;
...
jobQueue = malloc(nJobsMax*sizeof(JOB*));
filename=(char*)malloc(BUFSIZ);
while (fgets(filename,BUFSIZ,fp)!=NULL) {       
    trimLine(filename);        
    newjob = (JOB*)malloc(sizeof(JOB));
    newjob->filename = filename;
    filename=(char*)malloc(BUFSIZ);
    jobQueue[nJobs++] = newjob;
    if (nJobs > nJobsMax) 
        //possible buffer overflow need escape

}
free(filename);
fclose(fp);

more things:

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

really you don't need iterate all the string
example: fgetd output => text_text_text\r\n\0aq
' ' is character space values over this element are printer characters see ascii.

fgets() reads in at most one less than size characters from stream and stores them into the buffer pointed to by s. Reading stops after an EOF or a newline. If a newline is read, it is stored into the buffer. A terminating null byte (aq\0aq) is stored after the last character in the buffer.
source: fgets

strncpy is more recommended that strcpy because protect your code against buffer overflow.

The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated. If the length of src is less than n, strncpy() writes additional null bytes to dest to ensure that a total of n bytes are written.
source:strncpy

other solution to strcmp:

The strdup() function returns a pointer to a new string which is a duplicate of the string s. Memory for the new string is obtained with malloc(3), and can be freed with free(3). The strndup() function is similar, but only copies at most n bytes. If s is longer than n, only n bytes are copied, and a terminating null byte ('\0') is added.
source:strndup

Upvotes: 0

Sir Wellington
Sir Wellington

Reputation: 596

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.

Upvotes: 0

BlueDog
BlueDog

Reputation: 976

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.

Upvotes: 1

codymanix
codymanix

Reputation: 29520

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.

Upvotes: 0

Paul R
Paul R

Reputation: 213080

Change:

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

to:

#include <limits.h>

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

Upvotes: 1

dutt
dutt

Reputation: 8209

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

Upvotes: 1

Related Questions