String literals in c

Question

I'm having a bit of trouble with C and character arrays. I tried to search on SO but I didn't really see anything that could help me or I was not looking for the right thing.

I've this function:

char* readFile(char* file_path)
{
  FILE* fp = fopen(file_path, "r");
  size_t buffer = 4096;
  char ch;
  int index = 0;
  char* line = (char*)malloc(sizeof(char) * buffer);
  while( (ch = (char)fgetc(fp)) != EOF )
  {
      line[index] = ch;
      ++index;
      if(index == buffer -1)
      {
          buffer = buffer * 2;
          line = realloc(line, buffer);
      }
  }
  line = realloc(line, (sizeof(char) * index));
  line[index] = '\0';
  fclose(fp);
  return line;
}

Now, when I use this function in my code and try to free the reference, it causes a crash so I think I'm leaking memory somewhere.

char* data;

data = readFile("....");
free(data) <-- this line causes a crash!

I know if I didn't malloc inside the function the memory would go out of scope so that's a no go but using the current function as is, I get a crash. What I'm doing wrong?

edit After doing this read file

after the above readFile function I go into this function

GLuint getShaderProgram(const char* vshad, const char* fshad)
{
    GLuint vertexShader = glCreateShader(GL_VERTEX_SHADER);

    glShaderSource(vertexShader, 1, &vshad, NULL);
    glCompileShader(vertexShader);

    GLint success;
    GLchar infoLog[512];
    glGetShaderiv(vertexShader, GL_COMPILE_STATUS, &success);

    if(!success)
    {
        glGetShaderInfoLog(vertexShader, 512, NULL, infoLog);
        printf("ERROR VERTEX COMPILATION_FAILED %s\n",infoLog);
        SDL_Quit();
    }

    GLuint fragmentShader  = glCreateShader(GL_FRAGMENT_SHADER);
    glShaderSource(fragmentShader, 1, &fshad, NULL);
    glCompileShader(fragmentShader);

    glGetShaderiv(fragmentShader, GL_COMPILE_STATUS, &success);

    if(!success)
    {
        glGetShaderInfoLog(vertexShader, 512, NULL, infoLog);
        printf("ERROR FRAGMENT COMPILATION_FAILED %s\n",infoLog);
        SDL_Quit();
    }

    GLuint shaderProgram = glCreateProgram();

    glAttachShader(shaderProgram, vertexShader);
    glAttachShader(shaderProgram, fragmentShader);
    glLinkProgram(shaderProgram);

    glGetProgramiv(shaderProgram, GL_LINK_STATUS, &success);
    if(!success) {
        glGetProgramInfoLog(shaderProgram, 512, NULL, infoLog);
        printf("ERROR SHADER PROGRAM COMPILATION_FAILED %s\n",infoLog);
        SDL_Quit();
    }
    glDeleteShader(vertexShader);
    glDeleteShader(fragmentShader);

    return shaderProgram;
}

so the full code path is like this

char* vv = readFile("vshad.vs");
char* ff = readFile("fshad.fs");

sp = getShaderProgram(vv, ff);

free(vv);
free(ff);

I get crashes at the free(..), removing those and the program runs fine but I feel it's a memory leak.

Answer 1

Insufficient memory is allocated for the array. You have just allocate memory for the character array but not for the terminating \0 character. This leads to undefined behavior.

Please change the following line:

line = realloc(line, (sizeof(char) * index));

to

line = realloc(line, (sizeof(char) * (index+1)));

Also, change the datatype of ch to int from charsince the return type of fgetc() is int (not char). It is recommended to assign the returned values to an integer type variable.

When you read the value into a char instead of an int:

1. If char is unsigned, then you get an infinite loop as we never get EOF.
2. If char is signed, then 0xFF (i.e ÿ is accepted as EOF) is taken into account as EOF producing wrong results.

Also, check whether the functions worked as intended or not:

• Check file opening was successful or not. FILE* fp = fopen(file_path, "r"); if(fp != NULL){ //Do Stuff }

• Check whether memory was allocated or not

  char* tmpLine = realloc(line, (sizeof(char) * (index+1)));
  if (tmpLine != NULL)
  {
    line = tmp;
    line[index] = '\0';
  }
  else
  {
    //Handle insufficient memory
  }
  

Answer 2

In C it is important to check return values from functions that have return values. In your case you use realloc but don't bother to check the return value.

char* tmp = realloc(line, newbuffersize);
if (tmp != NULL)
{
  line = tmp;
}
else
{
  abort();
}

You should check if the file was opened properly, if it fails and you start using the null pointer in your calls you will get funny behavior.

Once leaving your while loop you downsize the buffer with a new realloc. That is fine but you need to add room for the ending \0 :

char* tmp = realloc(line, index + 1);
if (tmp != NULL)
{
  line = tmp;
  line[index] = '\0';
}

Generally speaking it is a bit ineffective to allocate as you do it, it would be more effective if first you check the size of the file (e.g. see fseek to the end and then do ftell then fseek to start) and then allocate a buffer with the same size (+1) before reading - if you not for some reason have draconian memory constraints or a massive big file.

Answer 3

change line = realloc(line, (sizeof(char) * index)); to

line = realloc(line, (sizeof(char) * (index+1)));

You are trying to realloc line to index size and next statement you are trying to store '\0' character in line[index] which is undefined behavior.