Simple and safe file reader for C As an exercise to improve my coding ability in C, I have attempted to write a simple and safe file reader. I believe this should be portable as well to major platforms (correct me if I'm wrong). For simplicity's sake I assume the file will fit in memory which is mostly true for my use case.

Please let me know where I can improve.

(Note: I've used goto because I think it makes the code flow better)

int read_file(const char* filename, char** buffer) {
    FILE* fp = fopen(filename, "r");
    if (!fp) {
        goto f_read_error;
    }

    ssize_t file_len;
    if (fseek(fp, 0L, SEEK_END) == 0) {
        file_len = ftell(fp);
        if (file_len == -1) {
            goto f_read_error;
        }
        if (fseek(fp, 0L, SEEK_SET) != 0) {
            goto f_read_error;
        }
        *buffer = (char*) malloc(sizeof(char) * (file_len + 1));

        int output = fread(*buffer, sizeof(char), file_len, fp);

        if (output != file_len) {
            goto f_read_error;
        };
        (*buffer)[file_len] = '\0';
    } else {
        f_read_error: ;
               char error_msg[1024];
               // let's trunc filenames larger than 256, to not overflow error_msg buffer
               char safe_filename[256];
               if (strlen(filename) &gt; 256) {
                   memcpy(safe_filename, filename, 256);
               }
               sprintf(error_msg, "Unable to read file : %s", safe_filename);
               perror(error_msg);
               if (buffer) {
                   free(buffer);
               }
               if (fp) {
                   fclose(fp);
               }
               return -1;
     }
     if (fp) {
        fclose(fp);
     }
     return 1;
}



int main(int argc, char** argv)  {
      char* source = NULL;
      int success = read_file("test.txt", &amp;source);
      if (success) {
            //read_file("test.txt", &amp;source); - a copy paste error, edited as a comment
            printf("%s\n", source);
            free(source);
      }
}
This code is not safe, and exhibits undefined behavior. I don't think you've actually tested it.

           char safe_filename[256];
           if (strlen(filename) &gt; 256) {
               memcpy(safe_filename, filename, 256);
           }
           sprintf(error_msg, "Unable to read file : %s", safe_filename);


Let's see what happens if filename is short. The test is false, so execution proceeds to the sprintf. It is constructing a string and is supposed to interpolate safe_filename... but that hasn't been initialized yet. Whatever garbage is in safe_filename will be copied into error_msg until it happens to hit a '\0', unless it gets a segfault first. It might even overflow the bounds of error_msg.

And what happens if filename is long? The test is true, so 256 characters of filename get copied into safe_filename. No terminating '\0' because memcpy() doesn't do that (and there wouldn't be room if it did). Execution proceeds to the sprintf. Again, it starts interpolating from safe_filename into the string, but the copying continues without stopping until it happens to hit a '\0' or segfaults. Same risk to error_msg.

How did this problem arise? You wrote code without testing it. Let me take a minute to pontificate about testing. You need to test your code. You need to test all your code. You need to be able to do the tests in an automated manner. That means your automatic testing harness needs to be able to cause each branch to be either taken or not taken.