The fread() function, as defined in the C Standard, subclause 7.21.8.1 [ISO/IEC 9899:2011], does not explicitly null-terminate the read character sequence.
Synopsis
size_t fread(void * restrict ptr, size_t size, size_t nmemb, FILE * restrict stream)Description
Thefreadfunction reads, into the array pointed to byptr, up tonmembelements
whose size is specified bysize, from the stream pointed to bystream.
Although the content of a file has a properly null-terminated character sequence, if nmemb is less than the total length of the characters, the fread() function will not read after nmemb characters. fread() will not append a null character to the end of the string being read to.
Noncompliant Code Example
Suppose we have a null-terminated character sequence in a file, and we need to extract a null-terminated byte string:
#include <stdio.h>
#include <stdlib.h>
int main (void) {
FILE *fp;
size_t size;
long length;
char *buffer;
fp = fopen("file.txt", "rb");
if (fp == NULL) {
/* Handle file open error */
}
/* Obtain file size */
if (fseek(fp, 0, SEEK_END) != 0) {
/* Handle repositioning error */
}
length = ftell(fp);
if (fseek(fp, 0L, SEEK_SET) != 0) {
/* Handle repositioning error */
}
/* Allocate memory to contain whole file */
buffer = (char*) malloc(length);
if (buffer == NULL) {
/* Handle memory allocation error */
}
/* size assigned here in some other code */
if (fread(buffer, 1, size, fp) < size) {
/* Handle file read error */
}
fclose(fp);
return 0;
}
When size is less than the total length of the file (file.txt), buffer is not properly null-terminated.
Compliant Solution
To correct this example, the size of buffer must be compared with the total length of the file to identify the erroneous case where size differs from length. At this point, it is up to the programmer to handle this case.
#include <stdio.h>
#include <stdlib.h>
int main (void) {
FILE *fp;
size_t size;
long length;
char *buffer;
fp = fopen("file.txt", "rb");
if (fp == NULL) {
/* Handle file open error */
}
/* Obtain file size */
if (fseek(fp, 0, SEEK_END) != 0) {
/* Handle repositioning error */
}
length = ftell(fp);
if (fseek(fp, 0L, SEEK_SET) != 0) {
/* Handle repositioning error */
}
/* Allocate memory to contain whole file */
buffer = (char*) malloc(length);
if (buffer == NULL) {
/* Handle memory allocation error */
}
/* ... Assign size here ... */
if (length != size) {
/* Handle case when size isn't the length of file */
}
/* ... Other code ... */
if (fread(buffer, 1, size, fp) < size) {
/* Handle file read error */
}
fclose(fp);
return 0;
}
Risk Assessment
When reading an input stream, the read character sequence is not explicitly null-terminated by the fread() function. Operations on the read-to buffer could result in overruns, causing abnormal program termination.
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
|---|---|---|---|---|---|
FIO17-C | Low | Likely | Medium | P6 | L2 |
Automated Detection
Tool | Version | Checker | Description |
|---|---|---|---|
| LDRA tool suite | 9.7.1 | 44 S | Enhanced enforcement |
Related Vulnerabilities
Search for vulnerabilities resulting from the violation of this guideline on the CERT website.
Related Guidelines
| SEI CERT C++ Coding Standard | VOID FIO20-CPP. Do not rely on an ending null character when using read() |
Bibliography
| [ISO/IEC 9899:2011] | Subclause 7.21.8.1, "The fread Function" |
11 Comments
David Svoboda
Hi, Glenn. This looks like a legit rule. I presume you mean to rewrite the C++ rule to work with the C language, so I'll reserve comments until you finish it.
I'll add one comment that in C 'read()' is a non-standard function (it is defined by POSIX but not C99), so you'll probably want to focus on the 'fread()' function which C99. (We have a separate section for POSIX-related rules.)
David Svoboda
The rule is valid; here are my comments:
Robert Seacord
Your risk analysis statement just restates the behavior of the function, it doesn't describe a risk.
Don't say c-string. Say "null terminated byte string".
Wrong format for references. Just lose the [1] and use [ISO/IEC 9899:1999]
Try to emulate the style of writing in the other, completed rules. For example, get rid of the "suppose" and "here we" stuff.
Your initial statement is weaker than your title. Go with as strong as a statement as you can correctly make.
Geoff Clare
In the compliant solution, shouldn't the
(length != size)check be done before thefread()rather than after it?David Svoboda
I suppose so; as that's more efficient (if length != size). Since size is computed separately by the program, I'm not sure that it makes a lot of difference when the comparison takes place, however. What do you think, Glenn?
Geoff Clare
My thinking was that with the current code, the part that assigns
sizewould need to include a check that(size <= length)to ensurefread()will not overflow the buffer. It doesn't make sense to do that check before thefread()and then a separate check for(length != size)after thefread(), rather than just a single(length != size)check before thefread().David Svoboda
I made the change. I agree; the size check should be made ASAP, as a good design principle.
Glenn Stroz
I believe my original thought was that the buffer allocated would be the length of the entire file. Thus there'd be no possibility of overflowing the buffer and you wouldn't need to check if size <= length.
Geoff Clare
The buffer size is the length that the file had at the time of the
fseek(fp, 0, SEEK_END). The file could grow larger by the time thefread()call is made.Frank Martinez
Perhaps I missed it; where is the assignment to the "size" variable in either example?
David Svoboda
In both code samples
sizeis supposed to be initialized in the comments just above the fread() call.There is a bigger problem in that both code samples violate FIO19-C. Do not use fseek() and ftell() to compute the size of a regular file. This technique is only guaranteed to work as expected for POSIX...both code samples will have surprising effects on Windows or other non-POSIX platforms.
There can also be overflow issues if this code runs on a platform where
sizeof(size_t) != sizeof(long).