0

I try to read a file to a buffer using a low level file descriptor. The method suppose to store the file data byte by byte to a char * buffer, parse that data, and then free the allocated buffer.

static void
parse_file(char path[11]) {
  int fd = open(path, O_RDONLY);
  if (fd == -1) {
    fprintf(stderr, "Failed to open a file '%s'", path);
    exit(errno);
  }
  char c;
  char *buffer = {0};
  unsigned int i = 0;
  while (read(fd, &c, 1)) {
    buffer = malloc(sizeof(char));  // Why *buffer want work here?
    *buffer = c;
    ++buffer;
    ++i;
  }
  buffer = malloc(sizeof(char));
  *buffer = '\0';
  buffer = &buffer[0];
  printf("Buffer data:\n%s\n", buffer);

  // Parse buffer data
  // ...
  
  buffer = &buffer[0];
  for (unsigned int j = 0; j <= i; ++j) {
    free(buffer);
    ++buffer;
  }
}

I come up with the above solution, but flycheck gives me a warning of unix.Malloc type:

Attempt to free released memory

How can I allocate the buffer char by char in a single loop?

siery
  • 467
  • 3
  • 20
  • 1
    `buffer = malloc(sizeof(char));` I'm not really sure what you are trying to do. That overwrites the pointer to the previous allocated memory and that previous allocation is thus lost forever. Similar with the loop that calls `free` - `++buffer` makes no sense. The memory pointer is stored in `buffer` and that is the only location that can be freed. `buffer++` is not an allocated address and cannot be freed. I think you may need to review pointers and try and simpler example first as the code shown just doesn't really make sense in multiple places and it is not clear what you are trying to do. – kaylum Jun 26 '22 at 11:35
  • 1
    Apart from anything else, why do you `malloc` for each `char` you read? And why `buffer = &buffer[0];`? – G.M. Jun 26 '22 at 11:36
  • 1
    ...and EOF is an int, so reading one byte into a char will never equal EOF:( – Martin James Jun 26 '22 at 11:41
  • So `buffer = malloc(sizeof(char))` does reallocate the char pointer. How else can I allocate the memory to the exact char instead of relocating the char pointer? `*buffer = malloc(sizeof(char))` is also wrong.. – siery Jun 26 '22 at 13:28
  • @G.M. So do I really need to read the file twice? First read the size of the buffer, then allocate the whole buffer, and then read the file again to store the data into the buffer? Is that the only way to do it? – siery Jun 26 '22 at 13:41
  • suggest reading about the function `realloc()` – user3629249 Jun 27 '22 at 17:46

2 Answers2

1

Construction like buffer = &buffer[0]; won't work. After the loop (and setting \0) buffer points to the last character (so to \0). Taking address of the 0th element will just give you address of the last element (as the buffer points). You cannot 'rewind' to the first character that way. When you call then free() you start freeing from your last element and then iterate over some memory region that was not allocated before.

asmie
  • 51
  • 4
-1

There are a three problems worth to mention with the above code.

  • *buffer = malloc(sizeof(char)) will not work as *buffer refers to a char type, not a char * (pointer).
  • buffer = &buffer[0] will not reset the buffer to it's first element. This can by done either by storing the address to the first element for later assignment, or rewinding backwards in a loop using pointer arithmetic.
  • Allocating memory byte by byte would be very inefficient. It is better to allocate the buffer after it's size is known.

I end up using a temporary automatic storage variable with a fixed size, and allocating the buffer's memory after the read loop:

static void
parse_file(char path[11]) {
  int fd = open(path, O_RDONLY);
  if (fd == -1) {
    fprintf(stderr, "Failed to open a file '%s'\n", path);
    exit(errno);
  }
  const int MAX = 50;
  char c;
  char *buffer = {0};
  char tmp[MAX];
  unsigned int i = 0;
  while (read(fd, &c, 1)) {
    tmp[i] = c;
    ++i;
  }
  
  buffer = malloc(sizeof(char) * i + 1);
  tmp[i] = '\0';
  strcpy(buffer, tmp);
  printf("Buffer data:\n%s\n", buffer);

  // Parse buffer data
  // ...
  
  free(buffer);
}
siery
  • 467
  • 3
  • 20