Reading a text file that may have embedded nulls

Question

I've always used fgets to read a file. However, I want to read a file that may have embedded \0. I thought of using ftell to query the size, but it doesn't seem to work on all files.

I've got a test file,

31 32 33 00 34 35 36 0A E2 82 AC 0D 0A 61

Here is my fgets.

#include <stdlib.h> /* EXIT */
#include <stdio.h>  /* printf perror fputc fread */
#include <string.h> /* strlen */
#include <errno.h>  /* errno */
#include <assert.h> /* assert */

int main(void) {
    char file[1000], *f = file, *a;
    const int granularity = 80;
    int is_done = 0;

    for( ; ; ) {
        /* Fail when contents bigger than the size;
         would be a good spot to use realloc. */
        if(granularity > sizeof file - (f - file))
            { errno = ERANGE; break; }
        if(!fgets(f, granularity, stdin))
            { if(ferror(stdin)) break; is_done = 1; break; }
        f += strlen(f);
    }

    for(a = file; a < f; a++) printf("%02hhX ", *a);
    fputc('\n', stdout);
    return is_done ? EXIT_SUCCESS : (perror("stdin"), EXIT_FAILURE);
}

Running this, (I'm on a UNIX-like machine,)

$ bin/fgets < test 
31 32 33 E2 82 AC 0D 0A 61 

Here is my fread.

#include <stdlib.h> /* EXIT */
#include <stdio.h>  /* printf perror fputc fread */
#include <errno.h>  /* errno */
#include <assert.h> /* assert */

int main(void) {
    char file[1000], *f = file, *a;
    const int granularity = 80;
    size_t read;
    int is_done = 0;

    for( ; ; ) {
        if(granularity > sizeof file - (f - file))
            { errno = ERANGE; break; }
        read = fread(f, 1, granularity, stdin);
        if(ferror(stdin)) break;
        assert(read >= 0 && read <= granularity);
        f += read;
        if(read != granularity) { is_done = 1; break; }
    }

    for(a = file; a < f; a++) printf("%02hhX ", *a);
    fputc('\n', stdout);
    return is_done ? EXIT_SUCCESS : (perror("stdin"), EXIT_FAILURE);
}

Running this,

$ bin/fread < test
31 32 33 00 34 35 36 0A E2 82 AC 0D 0A 61 

I would like to know if this is pedantically correct and how I improve.

Answer 1

I find your code hard to read, for the following reasons:

• the variable file is not really a file but a buffer
• the variable f (which usually stands for file) is a pointer into that buffer
• the variable a has a name which does not convey any meaning at all
• if the first if statement you indent the brace in the next line, and in the very last if statement the body is in the same line
• there are spaces missing in the code at places where I expect them, such as after an if or for
• the forever loop is usually written as for (;;), not for( ; ; )
• the main block of the code is inside the for loop, and there's not a single empty line in that part. This suggests that the whole block is doing a single thing with no possible interruption or logical break
• the comma operator is generally frowned upon
• is_done is not really about the work being done, it's more about being successful

Because of all the above reasons, I would write the code differently:

#include <errno.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>

int main(void) {
    char buf[1000];
    size_t buflen = 0;
    const size_t granularity = 80;

    while (true) {
        if (granularity > sizeof buf - buflen) {
            errno = ERANGE;
            break;
        }

        size_t nread = fread(buf + buflen, 1, granularity, stdin);
        if (ferror(stdin))
            break;

        buflen += nread;
    }

    for (size_t i = 0; i < buflen; i++)
        printf("%02hhX ", buf[i]);
    fputc('\n', stdout);

    if (ferror(stdin)) {
        perror("stdin");
        return EXIT_FAILURE;
    }
}

And here is what I changed:

• I renamed all variables to match their purpose
• I replaced the various pointers into the buffer with indexes
• I removed the is_done variable since the program should not print an error just because the file is a multiple of 80 bytes
• I moved the variable a to a smaller scope by declaring it inside the for loop where it is used; I also renamed it to i, since it is now an index instead of a pointer
• I replaced the comma operator with an if statement, since that is the form that is commonly used
• I included <stdbool.h> to have a boolean type and the constants true and false
• I replaced for (;;) with while (true), which is less magic
• I sorted the included headers alphabetically since for headers from the C standard library, the order doesn't matter
• I renamed the variable read to nread, to avoid possible conflicts with the POSIX function of the same name

Answer 2

Roland Illig provided an excellent review; there are a couple of points I'd like to add:

The standard header file <stdio.h> defines the macro/constant BUFSIZ. This macro was developed primarily for input and output buffers. In the original C it was defined as 1024, but now it varies from system to system, probably based on the file system blocking size.

It might have been better to define to define the character array using BUFSIZ.

The assert() macro is generally useful as a debugging tool and not included in production code. It will be optimized out of the code if the macro NDEBUG is defined.

The first time I read through the code, I missed all of the break; statements; it might be better if each statement is on a separate line.

Answer 3

Reading a text file that may have embedded nulls

fgets() is simply not the best tool to to do this.

Code that well handles text files with null characters employs *nix getline() or similar functions.

#include <stdlib.h>
#include <stdio.h>

int main(void) {
  size_t sz = 0;
  char *buf = NULL;
  unsigned long long line = 0;
  ssize_t count;
  char ch = '\n';
  while ((count = getline(&buf, &sz, stdin)) > 0) {
    printf("%llu", ++line);
    for (ssize_t i = 0; i < count; i++) {
      ch = buf[i];
      printf(" %02hhX:", ch);
    }
  }
  if (ch != '\n') {
    printf("\n");
  }
  free(buf);
  if (ferror(stdin)) {
    perror("stdin");
    return EXIT_FAILURE;
  }
  return EXIT_SUCCESS;
}

Answer 4

I would normally use ftell and allocate the buffer size needed, but I assume that you have some non-standard Unix Distro with specific environment or compiler settings, if you say that ftell doesnt work sometimes.

I provide an example with fix buffer size, just for demonstration. Buffer size reallocation and more error checking would probably be needed for production code.

Regarding code, generally, I always preferr staying generic and grouping code in functions. It's not only making code easier to read, but also makes a thinking process easier. And there's a reusabillity point too, of course.

So this would be my refactoring - suggestion (I didn't pay much attention to your error codes. And it could be that you have to put declarations on top, if you are using some older C compiler like C89 etc.):

#include <stdio.h>

#define GRANULARITY 80
#define MIN(a, b) (((a) < (b)) ? (a) : (b))

size_t read_file(FILE* fd,      // file descriptor
                 char *buf, // buffer
                 size_t size); // buffer size

int main(int argc, char* argv[])
{
    char buf[1000];
    size_t num_read = read_file(stdin, buf, sizeof(buf));

    if (num_read > 0)
    {
        for (int i = 0; i < sizeof(buf); ++i)
            printf("%02hhX ", buf[i]);
    }
    else if (errno == ERANGE)
    {
        // error - there are more bytes to read, handle it
    }   
    else if (ferror(fd))    
    {
         // stream error - handle it
    }

    return 0;
}

size_t read_file(FILE* fd,
                 char *buf,
                 size_t size)
{
    size_t num_read = 0;
    char *pos = buf;
    size_t n = 0;

    while ( (n = fread(pos, 1, MIN(GRANULARITY, buf+size-pos), fd)) > 0 )
    {
        num_read += n;
        pos += n;
    }

    if (!feof(fd))
        errno = ERANGE;   // buf too small - there are more bytes to read

    return num_read;

Edit: After reading some comments: changed the interface return type from ssize_t to size_t and moved the error-code checking responsibility to the caller.