-2

I want to read from a file using the read function and store the contains of that file in an array of chars. I know that the problem is that I store chars and I want to print a string. How can I obtain my string?

#include <stdio.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdlib.h>
#define MAX 1024



int main(int argc, char **argv)
{
    int f1;
    char *buf;
    char *rez = (char*)malloc(sizeof(char)* MAX);
    buf = (char*)malloc(sizeof(char)* MAX);
    if((f1=open("fis4.txt", O_RDONLY )) < 0)
    {
        printf("Error\n");
        exit(1);
    }
    int i = 0;
    while((read(f1, &buf,1))>0) 
    {
        rez[i]=buf[i];
        i++;
    }

    printf("%s", rez);

    close(f1);

return 0;

}
Donald Duck
  • 8,409
  • 22
  • 75
  • 99
  • A string is a sequence of `char`s. It just needs to be terminated by the NUL character `'\0'`. Is what you read NUL terminated? – StoryTeller - Unslander Monica Jan 15 '17 at 10:37
  • So if I append ' \0 ' to the last position, outside the while loop, like rez[i] ='\0' should it work? (still does not, so I think I do not understand properly) – DanielsQuestions Jan 15 '17 at 10:46
  • There has been a mistake in my code. It should have been rez[i] = buf[0\, since I read one char at a time from 1st pos; now it works – DanielsQuestions Jan 15 '17 at 11:43
  • 1
    1st start by taking the compiler's warnings serious. Fix the code until no more warnings are issued. Do not do this by blindly casting away warnings. – alk Jan 15 '17 at 12:02
  • You do not cast `malloc` in C. See https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc – Ed Heal Jun 18 '17 at 16:17
  • `if((f1=open("fis4.txt", O_RDONLY )) < 0)` - Where did you learn this? You should be checking for -1 – Ed Heal Jun 18 '17 at 16:19
  • Why are you ignoring `MAX` after the allocation? – Ed Heal Jun 18 '17 at 16:20

1 Answers1

1

There are several problems in your code. First of all, in the while loop, you need to test if i is greater than MAX, in which case you need to exit the loop to avoid buffer overflow. As it is now, if the file is longer than 1024 characters, it will crash because you're trying to write outside of the array. To avoid this, add this code to the while loop (after i++):

if(i + 1 >= MAX){
    break;
}

The break; command will exit the while loop.

Second of all, strings have to end with a null character. So you have to add a null character to each string after the while loop like this:

rez[i] = '\0';
buf[i] = '\0';

Note that when testing if i is greater than MAX, we did if(i + 1 >= MAX) instead of if(i >= MAX) to leave space for the null character.

Third of all, never use malloc without free, otherwise it will cause a memory leak. You have to free rez and buf at the end of your program like this:

free(rez);
free(buf);

An alternative to this which works in your case but not always would be to declare rez and buf as simple arrays without using malloc:

char buf[MAX];
char rez[MAX];

This works since MAX is a macro and not a variable.

Fourth of all, never leave a pointer uninitialized. Instead, initialize it to NULL:

char *buf = NULL;

However, in your case, it would be better to initialize it to malloc directly:

char *buf = (char*)malloc(sizeof(char) * MAX);

Note that in C, it is not required nor recommended to cast the result of malloc (although it is required in C++). The following code would therefore be better:

char *buf = malloc(sizeof(char) * MAX);

For more information, you can read this.

Fifth of all, as suggested in the comment, you have to check if the memory allocation from malloc failed, in which case you need to exit the program to avoid undefined behavior. If the memory allocation failed, the return value of malloc will be NULL. That gives this code, that you should place after malloc:

if(rez == NULL || buf == NULL){
    printf("Memory allocation failed.\n");
    exit(8);
}

The return value in this case would be 8 if you want to follow the standard return values. Of course, you can return what ever value you want if you don't want to follow the standard. If you want to follow the standard, the return value should also be 2 if the file isn't found (not 1 as in your code). You can also use the macros ERROR_FILE_NOT_FOUND (equal to 2) and ERROR_NOT_ENOUGH_MEMORY (equal to 8). These macros can be found in windows.h (although I don't know if you have this file since you're using Linux).

A version of your code that works would then be:

#include <stdio.h>
#include <stdlib.h>
#define MAX 1024

int main(int argc, char **argv){
    FILE *f1;
    char *buf = malloc(sizeof(char) * MAX);
    char *rez = malloc(sizeof(char) * MAX);
    if((f1=fopen("fis4.txt", "r" )) == NULL){
        printf("Error\n");
        exit(2);
    }
    if(rez == NULL || buf == NULL){
        printf("Memory allocation failed.\n");
        exit(8);
    }
    int i = 0;
    while((buf[i] = fgetc(f1)) != EOF){
        rez[i] = buf[i];
        i++;
        if(i + 1 >= MAX){
            break;
        }
    }

    rez[i] = '\0';
    buf[i] = '\0';
    printf("%s", rez);

    free(rez);
    free(buf);

    fclose(f1);

    return 0;
}
Donald Duck
  • 8,409
  • 22
  • 75
  • 99