Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
*.gcno
*.gch
*.o
*.a
*.pyc
/cppcheck
/cppcheck.exe
Expand Down
20 changes: 20 additions & 0 deletions lib/checkio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
// CVE ID used:
static const CWE CWE119(119U); // Improper Restriction of Operations within the Bounds of a Memory Buffer
static const CWE CWE398(398U); // Indicator of Poor Code Quality
static const CWE CWE474(474U); // Use of Function with Inconsistent Implementations
static const CWE CWE664(664U); // Improper Control of a Resource Through its Lifetime
static const CWE CWE685(685U); // Function Call With Incorrect Number of Arguments
static const CWE CWE686(686U); // Function Call With Incorrect Argument Type
Expand Down Expand Up @@ -111,6 +112,8 @@ namespace {
nonneg int op_indent{};
enum class AppendMode : std::uint8_t { UNKNOWN_AM, APPEND, APPEND_EX };
AppendMode append_mode = AppendMode::UNKNOWN_AM;
enum class ReadMode : std::uint8_t { READ_TEXT, READ_BIN };
ReadMode read_mode = ReadMode::READ_BIN;
std::string filename;
explicit Filepointer(OpenMode mode_ = OpenMode::UNKNOWN_OM)
: mode(mode_) {}
Expand Down Expand Up @@ -183,6 +186,7 @@ void CheckIO::checkFileUsage()
}
} else if (Token::Match(tok, "%name% (") && tok->previous() && (!tok->previous()->isName() || Token::Match(tok->previous(), "return|throw"))) {
std::string mode;
bool isftell = false;
const Token* fileTok = nullptr;
const Token* fileNameTok = nullptr;
Filepointer::Operation operation = Filepointer::Operation::NONE;
Expand Down Expand Up @@ -266,6 +270,9 @@ void CheckIO::checkFileUsage()
fileTok = tok->tokAt(2);
if ((tok->str() == "ungetc" || tok->str() == "ungetwc") && fileTok)
fileTok = fileTok->nextArgument();
else if (tok->str() == "ftell") {
isftell = true;
}
operation = Filepointer::Operation::UNIMPORTANT;
} else if (!Token::Match(tok, "if|for|while|catch|switch") && !mSettings->library.isFunctionConst(tok->str(), true)) {
const Token* const end2 = tok->linkAt(1);
Expand Down Expand Up @@ -321,10 +328,15 @@ void CheckIO::checkFileUsage()
f.append_mode = Filepointer::AppendMode::APPEND_EX;
else
f.append_mode = Filepointer::AppendMode::APPEND;
}
else if (mode.find('r') != std::string::npos &&
mode.find('t') != std::string::npos) {
f.read_mode = Filepointer::ReadMode::READ_TEXT;
} else
f.append_mode = Filepointer::AppendMode::UNKNOWN_AM;
f.mode_indent = indent;
break;

case Filepointer::Operation::POSITIONING:
if (f.mode == OpenMode::CLOSED)
useClosedFileError(tok);
Expand Down Expand Up @@ -357,6 +369,8 @@ void CheckIO::checkFileUsage()
case Filepointer::Operation::UNIMPORTANT:
if (f.mode == OpenMode::CLOSED)
useClosedFileError(tok);
if (isftell && f.read_mode == Filepointer::ReadMode::READ_TEXT && printPortability)
ftellFileError(tok);
break;
case Filepointer::Operation::UNKNOWN_OP:
f.mode = OpenMode::UNKNOWN_OM;
Expand Down Expand Up @@ -424,6 +438,12 @@ void CheckIO::seekOnAppendedFileError(const Token *tok)
"seekOnAppendedFile", "Repositioning operation performed on a file opened in append mode has no effect.", CWE398, Certainty::normal);
}

void CheckIO::ftellFileError(const Token *tok)
{
reportError(tok, Severity::portability,
"ftellTextModeFile", "ftell() result is unspecified when file is opened in mode \"t\"", CWE474, Certainty::normal);
}

void CheckIO::incompatibleFileOpenError(const Token *tok, const std::string &filename)
{
reportError(tok, Severity::warning,
Expand Down
2 changes: 2 additions & 0 deletions lib/checkio.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ class CPPCHECKLIB CheckIO : public Check {
void useClosedFileError(const Token *tok);
void fcloseInLoopConditionError(const Token *tok, const std::string &varname);
void seekOnAppendedFileError(const Token *tok);
void ftellFileError(const Token *tok);
void incompatibleFileOpenError(const Token *tok, const std::string &filename);
void invalidScanfError(const Token *tok);
void wrongPrintfScanfArgumentsError(const Token* tok,
Expand Down Expand Up @@ -141,6 +142,7 @@ class CPPCHECKLIB CheckIO : public Check {
"- Missing or wrong width specifiers in 'scanf' format string\n"
"- Use a file that has been closed\n"
"- File input/output without positioning results in undefined behaviour\n"
"- Using 'ftell' on a file opened in text mode\n"
"- Read to a file that has only been opened for writing (or vice versa)\n"
"- Repositioning operation on a file opened in append mode\n"
"- The same file can't be open for read and write at the same time on different streams\n"
Expand Down
56 changes: 56 additions & 0 deletions man/checkers/ftellTextModeFile.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# ftellModeTextFile

**Message**: ftell() result is unspecified when file is opened in mode "t".<br/>
**Category**: Portability<br/>
**Severity**: Style<br/>
**Language**: C/C++

## Description

This checker detects the use of ftell() on a file open in text (or translate) mode. The text mode is not consistent
in between Linux and Windows system and may cause ftell() to return the wrong offset inside a text file.

This warning helps improve code quality by:
- Making the intent clear that the use of ftell() in "t" mode may cause portability problem.

## Motivation

This checker improves portability accross system.

## How to fix

According to C11, the file must be opened in binary mode 'b' to prevent this problem.

Before:
```cpp
FILE *f = fopen("Example.txt", "rt");
if (f)
{
int position;
struct stat st;

// Wrong way to get the file size
fseek(f, 0, SEEK_END);
printf( "File size %d\n, ftell(f));
fclose(f);
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest such bad code:

Suggested change
}
// Wrong way to get file size:
FILE *f = fopen("Example.txt", "rt");
fseek(f, 0, SEEK_END);
printf("File size: %d\n", ftell(f));
fclose(f);


```

After:
```cpp

FILE *f = fopen("Example.txt", "rb");
if (f)
{
fseek(f, 0, SEEK_END);
printf( "Offset %d\n", ftell(f);
fclose(f);
}

```

## Notes

See https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fopen-wfopen?view=msvc-170

1 change: 1 addition & 0 deletions releasenotes.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ New checks:
- funcArgNamesDifferentUnnamed warns on function declarations/definitions where a parameter in either location is unnamed
- uninitMemberVarNoCtor warns on user-defined types where some but not all members requiring initialization have in-class initializers.
- fcloseInLoopCondition warns when fclose() is used as a while loop condition, which may skip the loop body or double-close the file handle.
- ftell() result is unspecified when file is opened in mode "t".

C/C++ support:
-
Expand Down
17 changes: 17 additions & 0 deletions test/testio.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class TestIO : public TestFixture {
TEST_CASE(fileIOwithoutPositioning);
TEST_CASE(seekOnAppendedFile);
TEST_CASE(fflushOnInputStream);
TEST_CASE(ftellCompatibility);
TEST_CASE(incompatibleFileOpen);

TEST_CASE(testScanf1); // Scanf without field limiters
Expand Down Expand Up @@ -727,6 +728,22 @@ class TestIO : public TestFixture {
ASSERT_EQUALS("", errout_str()); // #6566
}

void ftellCompatibility() {

check("void foo() {\n"
" FILE *f = fopen(\"\", \"rt\");\n"
" if (f)\n"
" {\n"
" extern long position;\n"
" fseek(f, 0, SEEK_END);\n"
" position = ftell(f);\n"
" fclose(f);\n"
" }\n"
"}\n", dinit(CheckOptions, $.portability = true));
ASSERT_EQUALS("[test.cpp:7:21]: (portability) ftell() result is unspecified when file is opened in mode \"t\" [ftellTextModeFile]\n", errout_str());
}


void fflushOnInputStream() {
check("void foo()\n"
"{\n"
Expand Down