whats wrong with strcpy()

Question

What is wrong with strcpy() in this code?

void process_filedata(char *filename)
{
  void* content;
  const char * buffer;
  char * temp;
  char * row;
  char * col;
  int lsize,buflen,tmp,num_scan; //num_scan - number of characters scanned
  int m=0,p=0,d=0,j=0; //m - machine, p - phase, d- delimiter, j - job

  FILE *file_pointer = fopen("machinetimesnew.csv","r");

  if(file_pointer == NULL)
  {
   error_flag =  print_error("Error opening file");

   if(error_flag) exit(1);
  }
  fseek(file_pointer, 0 ,SEEK_END);
  lsize = ftell(file_pointer);
  buflen = lsize;
  rewind(file_pointer);
 // content = (char*) malloc(sizeof(char)*lsize);
  fread(content,1,lsize,file_pointer);
  buffer = (const char*) content;
  strcpy(temp,buffer);
  row = strtok(temp,"\n");
  ...............
  ...............

I am getting a segmentation fault..

Answer 1

You're not allocating any space for temp. It's a wild pointer.

Answer 2

Nothing's wrong with strcpy. You haven't initialised temp.

Answer 3

you didn't allocate memory for temp

Answer 4

char * temp hasn't been initialized and you consequently haven't allocated any memory for it.

try:

temp = (char *)malloc(SIZE);

where SIZE is however much memory you want to allocate for temp

Answer 5

There are actually three segmentation faults here:

fread(content,1,lsize,file_pointer);
strcpy(temp,buffer);
row = strtok(temp,"\n");

The first one is fread() which is attempting to write to memory that does not yet exist as far as your process is concerned.

The second one is strcpy(), (expounding on the first) you are attempting to copy to a pointer that points to nothing. No memory (other than the pointer reference itself) has been allocated for temp, statically or dynamically.

Fix this via changing temp to look like this (allocating it statically):

char temp[1024];

Or use malloc() to dynamically allocate memory for it (as well as your other pointers, so they actually point to something), likewise for content. If you know the needed buffer size at compile time, use static allocation. If not, use malloc(). 'Knowing' is the subject of another question.

The third one is strtok() , which is going to modify temp en situ (in place), which it obviously can not do, since temp was never allocated. In any event, don't expect temp to be the same once strtok() is done with it. By the name of the variable, I assume you know that.

Also, Initializing a pointer is not the same thing as allocating memory for it:

char *temp = NULL; // temp is initialized
char *temp = (char *) malloc(size); // temp is allocated if malloc returns agreeably, cast return to not break c++

Finally, please get in the habit of using strncpy() over strcpy(), its much safer.

Answer 6

This piece of code intrigues me:

if(file_pointer == NULL)
{
   error_flag =  print_error("Error opening file");

   if(error_flag) exit(1);
}

Shouldn't you exit unconditionally if the file_pointer is NULL?

Answer 7

There's yet another mistake. fread does not add a nul character to the end of the buffer. That's because it only deals with arrays of bytes, not nul-terminated strings. So you need to do something like this:

content = malloc(lsize + 1);
fread(content,1,lsize,file_pointer);
content[lsize] = 0;
temp = malloc(lsize + 1);
strcpy(temp, content);

or this:

content = malloc(lsize);
fread(content,1,lsize,file_pointer);
temp = malloc(lsize + 1);
memcpy(temp, content, lsize);
temp[lsize] = 0;

(Also, in real code you should check the results of fread and malloc.)