FIO20-C. Avoid unintentional truncation when using fgets() or fgetws()

It shpuld be stated that an obvious alternative, to leave room in the buffer for one more character and when there was no newline transferred to tack a newline onto the end (then a terminator) is unsafe, because it quietly accepts an input that is not what was actually intended, with who knows what consequences.

See FIO37-C for Rose enforceability

C99 7.19.7.2 sez:

The fgets function reads at most one less than the number of characters specified by n from the stream pointed to by stream into the array pointed to by s. No additional characters are read after a new-line character (which is retained) or after end-of-file. A null character is written immediately after the last character read into the array.

/emphasis mine/

Doesn't this indicate that if a string in a file ends with newline, the newline will always be read?

I presume the purpose of this rule is to handle cases where a fgets does not read until the newline, either because the file reaches EOF, or the maximum chars are read, or because some other I/O error occurs.
I guess the rule should resolve this question.

Since this is flagged as unenforceable, should it be moved to a recommendation instead of a rule?

I don't think the CS will compile on MSVC because of where the following declaration appears:

size_t len = strlen(buffer);

That's good news, I guess.  We have avoided valid C99 declarations like this just to make sure this would compile on MSVC.  If this now works, I think it is reasonable to use this style of declaration.

One note...my Ubuntu box using gcc 4.6.3 neither recognizes 'bool' or 'false'. Even with -std=c99 or -std=c1x.
No other problems with this rule...er rec.

The last "compliant solution" with the expanding buffer is still not safe if the input can contain nullbytes. E.g. like this:

description=AAA\0AAAAAAAAAAAAAA

secret_token=1234

This would be parsed like "description=AAAsecret_token=1234", I think.

The expanding buffer example is broken.  temp[len] can never be '\n', though temp[len-1] can be - providing that len > 0.

temp[len] is the terminating \0, as len = strlen(temp).

In any case, using the temp buffer and strcat is unnecessarily convoluted.  strlen() and strcat() are expensive, as they both
require finding the end of the string.  And copying the data is unnecessary.

The usual portable solution looks like the following 32 lines of code.

#include <errno.h>
#include <stdlib.h>
#include <stdio.h>
 
char *fgetline( FILE *stream ) {
    size_t bufsize = 0,
           xpnsize = 80,
           idx = 0;
    char *buf = NULL;
    int c;
    while( (c = getc(stream)) != EOF && c != '\n' ) {
        if( idx + 2 > bufsize ) { /* Space for char & terminal \0 */
            char *nbuf;
            bufsize += xpnsize;
            nbuf = (char *) realloc( buf, bufsize ); /* Create or expand */
            if( nbuf == NULL ) {
                perror( "realloc" ); /* free(buf); return NULL; */
                abort();
            }
            buf = nbuf;
        }
        buf[idx++] = c;
    }
    if( c == EOF && idx == 0 ) { /* EOF with no data on last line */
        free( buf );
        return NULL;
    }
    if( bufsize == 0 ) { /* Empty line.  N.B. idx guaranteed to be 0 */
        buf = (char *) malloc( 1 );
        if( buf == NULL ) {
            perror( "malloc" ); /* return NULL; */
            abort();
        }
    }
    buf[idx] = '\0';
    return buf;
}

 
/* Sample usage.  */
 
char *line;
while( (line = fgetline( stdin )) != NULL ) {
    printf( "echo: %s\n", line );
    free(line);
}
exit(0);

David,

Thanks for addressing my comment.

I don't see why there's a premium on length; I prefer not to publish inefficient code.  Modifying code for "Efficiency" is often a source of bugs... I expect many people use the 'compliant' code as their solution, so it ought to be reasonably efficient.  And not encourage optimization....

Your current solution is 17 LOC,  My version is 24 (if you adopt the equivalent 'return NULL' on error alternative.)  Both counting lines with a statement, not blank or closing }.

(1.4x the line count, not 2x if you're picky.)

Are 7 LOC worth the inefficiency of processing the input three times as often as necessary?  (fgets() + strlen() + strcat()) 

Speaking of bugs:

Your new code can still index temp by -1 if strlen() returns zero.  One way it can do that if the input contains a \0.

strcat() and the manipulation of 'end' can an also be affected by \0 in the input.

fgets() doesn't deal gracefully with \0 in the input, but your new code exacerbates its issues.

You'll probably add a few LOC to deal with those boundary cases  If you don't switch to getc(), I predict that you'll pass over the input at least once more.

It should also be noted that a NULL return from fgets() or the aternatives doesn't only mean EOF.  It can mean an I/O error.  E.g. a buffer overrun on pasting from a terminal, or a disk or network failure. 

Failure to check errno or ferror() on an error return from fgets() is also a source of truncation errors.  I mentioned this in my first NOTE, but it may not have stood out.

Also, using getline() or either of the expanding buffer alternatives is low effort - why do you classify this as medium?  For the caller, it's actually more convenient.

This article says that a tool suite checks for compliance.  I'm not familiar with the tool, and it's not obvious how it can verify compliance, even with fairly extensive code analysis.  A check for missing newline can be a considerable distance from the fgets() in real code.  You run into the halting problem. 

In any case, the tool needs to be updated with the result of our discussion.

I noticed a vulnerability in the expanding buffer solution: the reallocated buffer in the first iteration is uninitialized (not guaranteed to be a string of length 0), so running strcat() on it is undefined behavior and may cause a buffer overflow.

In addition, the initial calculation of the full_size variable involves subtracting two null pointers (end and ret), which seems to be undefined behavior in C, though defined in C++. I think it would be a better idea to keep full_size a persistent variable between loop iterations, since the end pointer is only being used to keep track of the buffer size in a rather roundabout way.

One way to fix both of these issues would be to use strcpy() rather than strcat(), making use of the fact that the previous length of the string is already known (also improving efficiency). One such solution follows (note that full_size has been renamed to full_length to indicate it is the string length and not the buffer size):

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
 
char *get_filled_buffer(void) {
  char temp[32];
  char *ret = NULL;
  size_t full_length = 0;
  
  while (fgets(temp, sizeof(temp), stdin)) {
    size_t len = strlen(temp);
    char *r_temp = realloc(ret, full_length + len + 1);
    if (r_temp == NULL) {
      break;
    }
    ret = r_temp;
    strcpy(ret + full_length, temp); /* concatenate */
    full_length += len;
  
    if (feof(stdin) || temp[len-1] == '\n') {
      return ret;
    }
  }
 
  free(ret);
  return NULL;
}