Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
    
If your code is called by a kernel driver, it certainly shouldn't be calling things like malloc and fread. Does that even work? Does this code run in user address space or kernel address space? –  Greg Hewgill Aug 29 at 0:27
    
It DOES in fact run in USER space, but it's called by a Kernel driver. It's used in a virus scanner, so it only has milliseconds to a second or two to deal with a single file. But to answer your question, YES, this absolutely runs IN USER SPACE... –  LarryF Aug 29 at 0:34
    
As this custom code calculates the length of the string, maybe also pass that back upon function completion should the application needed it. –  chux Aug 29 at 21:23

4 Answers 4

up vote 4 down vote accepted

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;
}
share|improve this answer
    
mygets() signature differs importantly from char *fgets(char * restrict s, int n, FILE * restrict stream); in that it uses int n. IMO, both should be size_t. –  chux Aug 29 at 20:20
    
In the Alternate Implementation, // If fgets() didn't find a newline, then end is NULL, and it read does not take into account a line at the end of a stream that does not a have a '\n'. –  chux Aug 29 at 21:26
  • 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.

share|improve this answer
    
For most compilers, will the NULL compared to a char data type result in a 32 bit comparison, say, out of EAX vs. a short comparison out of AX? –  LarryF Aug 28 at 21:15
1  
Comparing a char to NULL on a 32-bit machine probably would do a 32-bit comparison (this is easy to check by looking at the compiler output). However, whether or not this compiles to code that happens to work doesn't change the fact that your intent is to compare a char and a char, not a char and a void *. –  Greg Hewgill Aug 28 at 21:47
    
Ok. I understand, that makes perfect sense, since I'm pretty anal about my code being correct! Thanks! –  LarryF Aug 28 at 22:05

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;
}
share|improve this answer
    
Good call on delegating to fgets for the actual reading work. No need to duplicate that function which already exists and is tested! –  Greg Hewgill Aug 28 at 21:50
    
Without the memset OP had, the second line of your function invokes undefined behavior if buf is not already initialized, and does the wrong thing even if buf was initialized but contains old content. –  R.. Aug 29 at 2:38
    
@R.. Can you elaborate on how UB could be invoked? –  vnp Aug 29 at 5:51
    
@vnp: The memchr can access uninitialized memory. Perhaps you're assuming a '\n' will occur before any uninitialized part of the buffer is reached, but that's false if you reach EOF without a newline. The only way to fix it is to memset the buffer to all '\n' before fgets and reject the '\n' as "not actual file content" if it doesn't have a 0 byte immediately following it. –  R.. Aug 29 at 6:13
    
@R..: good point. Not a UB, but just a plain bug. Fixing. –  vnp Aug 29 at 17:14
  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.

share|improve this answer
    
Good eye there chux... I cannot believe I missed the check for a failed memory allocation, which, as I mentioned, w/ the Kernel involved should have been a no brainier. Besides all of the other major flaws pointed out, I still look upon the lack of memory allocation as a HUGE problem, because it should become habit to us C programmers to check after a malloc(), just as it is habit to type {, and } at the start and end of a scope! –  LarryF Aug 30 at 5:00

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.