NULL-eating / elimination version of fgets() in C

Question

I've come up with the following routine to read data from a file stream, but I don't want any NULLs in the stream to terminate the string early. This function will read in a "line", and allow it to be processed with normal C string functions. Does anyone see any inherent problems or potential bugs in this routine?

char *my_fgets(char *Buf, int iMaxCount, FILE *fp)
{
    // Handle basic failures.
    if(!Buf) return NULL;
    if(iMaxCount < 1) return NULL;
    if(!fp) return NULL;

    // Create a buffer on the heap for our data to be read in
    char *lbuf = (char*)malloc(iMaxCount);
    char *p = lbuf;

    // Zero out the buffer, no stray chars please.
    memset(Buf, 0x00, iMaxCount);

    // Where are we at in the file?
    int b = ftell(fp);

    // Read in the buffer, up to iMaxCount-1.  Leave one char for a final NULL.
    size_t read = fread((void*)&lbuf[0], sizeof(char), iMaxCount-1, fp);
    lbuf[iMaxCount-1] = NULL;

    // Failed.
    if(!read) {
        free(lbuf);
        return NULL;
    }
    // Walk the buffer, looking for a newline char.  Once we find it, we know we are at the end of the line, 
    // but we don't want to read past the number of bytes we originally read, in case we did not hit a newline.
    size_t left = read;
    while((left) && *(p++) != '\n') left--;

    // Copy the data we read to the return Buffer, up to the newline char.
    memcpy(Buf, lbuf, p - lbuf);
    // Set the file pointer to the newline char + 1
    fseek(fp, b+(p - lbuf), SEEK_SET);

    // Setup how many bytes we have left in the buffer to look at.
    left = (p - lbuf);
    p = Buf;    

    while(left)
    {
        // Walk the buffer, looking for NULL chars.
        if(*(p++) == NULL) {
            char *p1;
            // We hit a NULL, so shift the buffer left, eat the NULL, and continue our search.
            for(p1 = p-1;p1 < (Buf + (iMaxCount-1));p1++) p1[0] = p1[1];
            // We ate one char, so we need to step back one byte, and continue our search.  This handles a case where two NULLs may be back to back.
            p--;
        }
        left--;
    } 

    // If our last char is not a NULL, we need to set it to NULL to properly terminate the string.
    if((p > Buf) && *(p-1) != 0x0a && *p != NULL) *(--p) = 0x00;

    // Free our work buffer, and return the new 'clean' data.
    free(lbuf);
    return Buf;
}

It might of importance to note that this code is called by a Kernel driver, and MUST be fast. Speed is totally of the essence here, which is why this is all in one function, and uses calls like fread() over fgets() to get a chunk of data in one shot. That might not matter in the final analysis, but I thought I would mention it.

Answer 1

In this answer I'm aggregating and elaborating on the answers already given, and then I provide a heavily-commented alternate version of your function.

NULL vs. '\0'

You shouldn't be using NULL the way that you are. It is true that NULL is almost always defined as

#define NULL 0

and your code above will work. However, the purpose of NULL in C code is to represent an invalid pointer, and so you should only use it in that case. The ASCII byte 0x00 is sometimes referred to as NUL, but this is only coincidentally similar to NULL in C.

The correct notation for a 0x00 byte in C (especially in the context of C-strings) is '\0'. Just like NULL, this is a shorthand for 0, but it conveys your intention much better (and saves you from running into compilation errors on the odd system where NULL is (void *)0).

On Rewriting Standard Functions

In your code, you provided (effectively) your own implementations of the standard functions fgets() and memchr(). You should always use the standard functions, unless you are absolutely sure that your version will perform better.

Standard library routines have almost always been optimized like crazy, and have probably considered/discovered edge-cases that you might not. They've also been heavily tested in production.

Cases where you might legitimately reimplement standard routines include

• Their solution is too general, and your specific use-case allows for optimizations that the general case doesn't.

• They don't exist, you're in the kernel (which it sounds like you might be?), or some other scenario.

• You have constraints that the standard implementations don't conform to.

An Alternate Implementation

So here's an implementation which uses all of the above advice and provides a better algorithm for stripping the '\0's from your buffer.

#include <stdio.h>  // fgets()
#include <string.h> // memchr()

// my_fgets() behaves exactly like the standard fgets(), except that it removes
// any '\0' bytes from the data it reads, and then adds a single '\0' byte to the
// end.
char *my_fgets(char *buf, size_t iMaxCount, FILE *fp) {
  // Delegate the actual reading to fgets(). The people who implemented it are
  // almost definitely doing it in the fastest way possible.
  if (fgets(buf, iMaxCount - 1, fp) == NULL) {
    return NULL;
  }

  // Use the standard memchr() function to find the first occurrance of a byte 
  // in a buffer. The standard function is probably optimized like crazy.
  char *end = memchr(buf, '\n', iMaxCount - 1); 
  // We want end to point just past the end of the data read by fgets(), so that
  // we can use it as an upper bound in the for-loop below.
  if (end == NULL) {
    // If fgets() didn't find a newline, then end is NULL, and it read in a
    // whole (iMaxCount - 1) bytes of data. So end needs to point to the very
    // last byte of buf.
    end = buf + (iMaxCount - 1); 

  } else {
    // Otherwise we found a '\n', that was the last byte read by fgets(), and
    // memchr() already found it for us, and returned a pointer to it which is
    // currently stored in end. We just have to advance end by one (so that
    // we're pointing just past the data we read).
    end++;

  }
  // In the following loop, i_ptr is used to read each of the input bytes, and
  // o_ptr is used to write out only the ones which are not '\0'.
  char *i_ptr, *o_ptr;

  // Note that we increment i_ptr every time, but we only increment o_ptr after
  // we find a valid non-'\0' character.
  for (i_ptr = o_ptr = buf; i_ptr < end; i_ptr++) {
    if (*i_ptr != '\0') {
      // *i_ptr isn't a '\0', so we keep it in the array
      *o_ptr++ = *i_ptr;
    }
  }

  // We've advanced o_ptr past all of the non-'\0' elements of the buffer, so
  // now it points exactly past the end, right where we want our '\0' byte.
  *o_ptr = '\0';

  // fgets() is specified as returning the input buffer, so we do that too.
  return buf;
}

Answer 2

• NULL is not a char. NULL is a pointer and should not be used when a char is needed. Use '\0' or 0x00.

• The temporary buffer that you malloc is not needed. You can read directly into Buf and clean it out (memset the remainder to 0x00) as needed.

• I would use read == 0 instead of !read.

• The dual use of left is a bit odd. First it means one thing (number of chars left to check) then it means something else (number of chars read). Use a different variable.

vnp already suggested improvement of the null-eating part.

Answer 3

There's absolutely no need for an intermediate (dynamic) buffer. Everything you do can be done directly in the client buffer (Buf).

I don't see a reason for fread-ing data. fgets works as good, and spares you from calling ftell and fseek.

The way you remove null chars is suboptimal. I'd keep count of null chars encountered, and shift non-null chars by that count, in a single loop. In any case, I'd factor removal code into a separate function (No Raw Loops rule strikes again).

Looking for a newline is better be done with a standard memchr().

That said, the main code condenses to

char * fgets_and_remove_nulls(char * buf, size_t count, FILE * fp)
{
    memset(buf, 0, size);
    if(fgets(buf, size, fp) == NULL) return NULL;
    char * end = memchr(buf, '\n', size);
    if (end == NULL) end = buf + size;
    remove_null_chars(buf, end);
    return buf;
}

Answer 4

1. The ftell()/fseek() approach fails in 2 places. Say the input was a serial device, either ftell()/fseek() may simple fail. For a text file, fseek(fp, b+(p - lbuf), SEEK_SET); fails the following:

   For a text stream, either offset shall be zero, or offset shall be a value returned by an earlier successful call to the ftell function on a stream associated with the same file and whence shall be SEEK_SET. C11dr §7.21.9.2 4

2. Various methods have difficulty when the last line of the stream does not have a '\n'. Rather than pre-load the array with '\0', pre-load the buffer with '\n'. Then use 1 of methods.

   A. Look for the 1st '\n'. This '\n' should be either followed by '\0' or another '\n'. From each case, the real buffer length can be determined. @R.. hints at this.

   B. Search from the buffer's end toward the beginning. Code should only encounter '\n' or '\0'. Upon reaching '\0', code has the true length. Although likely not a real concern, this method works even if alternate end-of-lines inject a \n not at the end of line. (see #5).

3. Curious that code checks for NULL input parameters, but does not check memory allocation.

   char *lbuf = (char*)malloc(iMaxCount);  // cast not needed
   if (lbuf == NULL) return NULL;  // Handle out of memory
   

4. The allocation of lbuf is not needed. Simple use buf.

5. For text streams, since code is concerned with embedded '\0', code may also need to cope with embedded alternate line endings for text streams. Example: Take the 3 popular line endings: '\r', '\n',"\r\n". The expected kind one will readily translate into '\n', but what of the other 2?

6. NULL is used in a questionable context. - as others have said. My takes of the 3 nulls:

   A. In C, NULL in the null pointer. It may/may not have a binary pattern of all zeros. if (!NULL) is always true.

   B. In C, (char) '\0' in the null character. It has the value of 0. I think it must have a binary pattern of all zeros.

   C. NUL is the ASCII code with the value of 0. ASCII is commonly used in C as the character coding set.

7. As others have said, the removal of embedded '\0' is inefficient: O(n*n). A single pass will do.