Modify rule S2095: Expand and adjust for LaYC
## Review A dedicated reviewer checked the rule description successfully for: - [ ] logical errors and incorrect information - [ ] information gaps and missing content - [ ] text style and tone - [ ] PR summary and labels follow [the guidelines](https://github.com/SonarSource/rspec/#to-modify-an-existing-rule)
This commit is contained in:
Philipp Dominik Schubert
GitHub
parent
6c7a0f2fea
commit
1595dcd062
@ -1,43 +1,112 @@
|
||||
A function call to ``++fopen++`` or ``++open++`` must be matched with a call to ``++fclose++`` or ``++close++``, respectively.
|
||||
|
||||
== Why is this an issue?
|
||||
|
||||
A call to the ``++fopen()++``/``++open()++`` function must be matched with a call to ``++fclose()++``/``++close++``. Otherwise, you run the risk of using up all the OS's file handles, which could lock up not just your program but potentially everything on the box.
|
||||
The standard C library provides ``++fopen++`` and the system call ``++open++`` to open and possibly create files.
|
||||
Each call to one of these functions must be matched with a respective call to ``++fclose++`` or ``++close++``.
|
||||
|
||||
=== Noncompliant code example
|
||||
Failing to close files that have been opened may lead to using up all of the OS's file handles.
|
||||
|
||||
[source,cpp]
|
||||
|
||||
== What is the potential impact?
|
||||
|
||||
If a program does not properly close or release file handles after using them, it will leak resources.
|
||||
In that case, the program will continue to hold onto file handles even when they are no longer needed, eventually exhausting the available file handles.
|
||||
|
||||
If a program has run out of file handles and tries to open yet another file, the file opening operation will fail.
|
||||
This can result in errors or unexpected behavior in the program.
|
||||
|
||||
The program may not be able to read or write to files anymore, which can cause data loss, corruption, or incomplete operations.
|
||||
In some cases, when a program runs out of file handles, it may crash or hang indefinitely.
|
||||
This can happen if the program does not handle the error condition properly or if it enters an infinite loop trying to open files, for instance.
|
||||
In the worst case, a resource leak can lock up everything that runs on the machine.
|
||||
|
||||
|
||||
== How to fix it
|
||||
|
||||
Make sure that each call to ``++fopen++`` and ``++open++`` has a matching call to ``++fclose++`` and ``++close++``, respectively.
|
||||
|
||||
=== Code examples
|
||||
|
||||
==== Noncompliant code example
|
||||
|
||||
[source,cpp,diff-id=1,diff-type=noncompliant]
|
||||
----
|
||||
int fun() {
|
||||
FILE *f = fopen("file", "r");
|
||||
if (f == NULL) {
|
||||
return -1;
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
|
||||
int process_file(int print) {
|
||||
FILE *f = fopen("example.txt", "r");
|
||||
if (!f) {
|
||||
perror("fopen() failed");
|
||||
return 1;
|
||||
}
|
||||
// ...
|
||||
return 0; // Noncompliant, file f has not been closed
|
||||
|
||||
if (print) {
|
||||
char buffer[256];
|
||||
while (fgets(buffer, 256, f)) {
|
||||
printf("%s", buffer);
|
||||
}
|
||||
}
|
||||
|
||||
return 0; // Noncompliant: file `f` has not been closed
|
||||
}
|
||||
----
|
||||
|
||||
=== Compliant solution
|
||||
==== Compliant solution
|
||||
|
||||
[source,cpp]
|
||||
[source,cpp,diff-id=1,diff-type=compliant]
|
||||
----
|
||||
int fun() {
|
||||
FILE *f = fopen("file", "r");
|
||||
if (f == NULL) {
|
||||
return -1;
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
|
||||
int process_file(int print) {
|
||||
FILE *f = fopen("example.txt", "r");
|
||||
if (!f) {
|
||||
perror("fopen() failed");
|
||||
return 1;
|
||||
}
|
||||
// ...
|
||||
|
||||
if (print) {
|
||||
char buffer[256];
|
||||
while (fgets(buffer, 256, f)) {
|
||||
printf("%s", buffer);
|
||||
}
|
||||
}
|
||||
|
||||
fclose(f);
|
||||
return 0;
|
||||
return 0; // Compliant: file `f` has been closed
|
||||
}
|
||||
----
|
||||
|
||||
=== Going the extra mile
|
||||
|
||||
Using {cpp}'s _RAII_ idiom can mitigate unmatched calls to ``++fopen++`` and ``++open++``.
|
||||
|
||||
include::../../../shared_content/cfamily/c_file_io_raii_extra_mile.adoc[]
|
||||
|
||||
With the design shown above, calls to ``++fopen++`` will be automatically matched with a corresponding call to ``++fclose++`` by default.
|
||||
The associated file will be automatically closed when the ``++File++`` typed file handle object goes out of scope and its destructor is called.
|
||||
However, it is still possible to leak a resource, if ``++File++``'s function member ``++File::release++`` is used inappropriately.
|
||||
If this is a concern, this function member should be removed.
|
||||
|
||||
If falling back to low-level file operations is not necessary, one should prefer ``++std::fstream++``.
|
||||
|
||||
|
||||
== Resources
|
||||
|
||||
* https://cwe.mitre.org/data/definitions/459[MITRE, CWE-459] - Incomplete Cleanup
|
||||
* https://cwe.mitre.org/data/definitions/772[MITRE, CWE-772] - Missing Release of Resource after Effective Lifetime
|
||||
* https://wiki.sei.cmu.edu/confluence/x/vjdGBQ[CERT, FIO04-J.] - Release resources when they are no longer needed
|
||||
* https://wiki.sei.cmu.edu/confluence/x/QtUxBQ[CERT, FIO42-C.] - Close files when they are no longer needed
|
||||
* https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html[Try With Resources]
|
||||
=== Standards
|
||||
|
||||
* CERT - https://wiki.sei.cmu.edu/confluence/x/vjdGBQ[FIO04-J. Release resources when they are no longer needed]
|
||||
* CERT - https://wiki.sei.cmu.edu/confluence/x/QtUxBQ[FIO42-C. Close files when they are no longer needed]
|
||||
* CWE - https://cwe.mitre.org/data/definitions/459[459 Incomplete Cleanup]
|
||||
* CWE - https://cwe.mitre.org/data/definitions/772[772 Missing Release of Resource after Effective Lifetime]
|
||||
|
||||
=== Related rules
|
||||
|
||||
* S3588 ensures that ``++FILE*++`` typed variables are not accessed after the associated file has been closed
|
||||
|
||||
|
||||
|
||||
ifdef::env-github,rspecator-view[]
|
||||
|
||||
|
||||
@ -38,5 +38,5 @@
|
||||
"defaultQualityProfiles": [
|
||||
"Sonar way"
|
||||
],
|
||||
"quickfix": "unknown"
|
||||
"quickfix": "infeasible"
|
||||
}
|
||||
|
||||
@ -46,83 +46,6 @@ The application might just crash, but in the worst case, the application may app
|
||||
|
||||
Do not use the value of a pointer to a ``++FILE++`` object after the associated file has been closed.
|
||||
|
||||
=== Going the extra mile
|
||||
|
||||
Using {cpp}'s _RAII_ idiom can mitigate these "double-close" issues.
|
||||
|
||||
Following this idiom, one would create a class that manages the underlying file by opening it when the object is constructed and closing it when the object is destroyed, effectively using a constructor-destructor pair as a "do-undo"-mechanism.
|
||||
|
||||
An exemplary class that manages a pointer to a file is shown in what follows.
|
||||
|
||||
[source,cpp]
|
||||
----
|
||||
#include <cstdio>
|
||||
#include <fstream>
|
||||
#include <string>
|
||||
#include <utility>
|
||||
|
||||
// Although `std::fstream` should be preferred, if available, a file stream
|
||||
// managed by this `File` class cannot suffer from "double-close" issues.
|
||||
class File {
|
||||
FILE *f;
|
||||
|
||||
public:
|
||||
// Opens file stream on construction.
|
||||
File(std::string const &path, std::string const &modes)
|
||||
: f(fopen(path.c_str(), modes.c_str())) {
|
||||
if (!f) {
|
||||
throw std::ios_base::failure("fopen() failed");
|
||||
}
|
||||
}
|
||||
// Will close the file stream upon destruction.
|
||||
~File() {
|
||||
// Here we are fine with `std::terminate` being called here in case `close`
|
||||
// throws and exception.
|
||||
close();
|
||||
}
|
||||
// Allow only one owner of a file, disallow copy operations.
|
||||
File(const File &other) = delete;
|
||||
File &operator=(const File &other) = delete;
|
||||
// Moving a file to a different scope shall be allowed.
|
||||
File(File &&other) : f(std::exchange(other.f, nullptr)) {}
|
||||
File &operator=(File &&other) {
|
||||
if (this != &other) {
|
||||
// In case of non-self-assignment, close the currently managed file and
|
||||
// "steal" the other's file.
|
||||
close();
|
||||
f = std::exchange(other.f, nullptr);
|
||||
}
|
||||
return *this;
|
||||
}
|
||||
// Allow file to be closed explicitly.
|
||||
void close() {
|
||||
if (f != nullptr && fclose(std::exchange(f, nullptr)) == EOF) {
|
||||
throw std::ios_base::failure("fclose() failed");
|
||||
}
|
||||
}
|
||||
// Allow access to underlying file via `f`.
|
||||
FILE *handle() { return f; }
|
||||
// Release `f`, i.e., stop managing it.
|
||||
FILE *release() { return std::exchange(f, nullptr); }
|
||||
};
|
||||
|
||||
void file_user() {
|
||||
File fh{"example.txt", "r"};
|
||||
FILE *f = fh.handle();
|
||||
// Use `f` for the desired file operation(s).
|
||||
//
|
||||
// The file stream managed by `fh` will be automatically closed when `fh` goes
|
||||
// out of scope at the end of this function.
|
||||
}
|
||||
----
|
||||
|
||||
With the design shown above, it is still possible to "double-close" a file by passing the raw ``++FILE++`` pointer obtained by a call to ``++File::handle++`` to ``++fclose++`` (e.g. ``++fclose(f.handle())++``).
|
||||
However, this design reduces the risk of such occurrence by eliminating the need for manually closing files.
|
||||
If even the reduced possibility of "double-close" is still a concern, the function member ``++File::handle++`` should be removed and any required file operations should be wrapped by the ``++File++`` class.
|
||||
|
||||
If falling back to low-level file operations is not necessary, one should prefer ``++std::fstream++``.
|
||||
|
||||
|
||||
=== Code examples
|
||||
|
||||
==== Noncompliant code example
|
||||
@ -180,6 +103,18 @@ int process_file(int print) {
|
||||
}
|
||||
----
|
||||
|
||||
=== Going the extra mile
|
||||
|
||||
Using {cpp}'s _RAII_ idiom can mitigate these "double-close" issues.
|
||||
|
||||
include::../../../shared_content/cfamily/c_file_io_raii_extra_mile.adoc[]
|
||||
|
||||
With the design shown above, it is still possible to "double-close" a file by passing the raw ``++FILE++`` pointer obtained by a call to ``++File::handle++`` to ``++fclose++`` (e.g. ``++fclose(f.handle())++``).
|
||||
However, this design reduces the risk of such occurrence by eliminating the need for manually closing files.
|
||||
If even the reduced possibility of "double-close" is still a concern, the function member ``++File::handle++`` should be removed and any required file operations should be wrapped by the ``++File++`` class.
|
||||
|
||||
If falling back to low-level file operations is not necessary, one should prefer ``++std::fstream++``.
|
||||
|
||||
|
||||
== Resources
|
||||
|
||||
|
||||
65
shared_content/cfamily/c_file_io_raii_extra_mile.adoc
Normal file
65
shared_content/cfamily/c_file_io_raii_extra_mile.adoc
Normal file
@ -0,0 +1,65 @@
|
||||
Following this idiom, one would create a class that manages the underlying file by opening it when the object is constructed and closing it when the object is destroyed, effectively using a constructor-destructor pair as a "do-undo"-mechanism.
|
||||
|
||||
An exemplary class that manages a pointer to a file is shown in what follows.
|
||||
|
||||
[source,cpp]
|
||||
----
|
||||
#include <cstdio>
|
||||
#include <fstream>
|
||||
#include <string>
|
||||
#include <utility>
|
||||
|
||||
// Although `std::fstream` should be preferred, if available, a file stream
|
||||
// managed by this `File` class cannot suffer from "double-close" issues.
|
||||
class File {
|
||||
FILE *f;
|
||||
|
||||
public:
|
||||
// Opens file stream on construction.
|
||||
File(std::string const &path, std::string const &modes)
|
||||
: f(fopen(path.c_str(), modes.c_str())) {
|
||||
if (!f) {
|
||||
throw std::ios_base::failure("fopen() failed");
|
||||
}
|
||||
}
|
||||
// Will close the file stream upon destruction.
|
||||
~File() {
|
||||
// Here we are fine with `std::terminate` being called here in case `close`
|
||||
// throws and exception.
|
||||
close();
|
||||
}
|
||||
// Allow only one owner of a file, disallow copy operations.
|
||||
File(const File &other) = delete;
|
||||
File &operator=(const File &other) = delete;
|
||||
// Moving a file to a different scope shall be allowed.
|
||||
File(File &&other) : f(std::exchange(other.f, nullptr)) {}
|
||||
File &operator=(File &&other) {
|
||||
if (this != &other) {
|
||||
// In case of non-self-assignment, close the currently managed file and
|
||||
// "steal" the other's file.
|
||||
close();
|
||||
f = std::exchange(other.f, nullptr);
|
||||
}
|
||||
return *this;
|
||||
}
|
||||
// Allow file to be closed explicitly.
|
||||
void close() {
|
||||
if (f != nullptr && fclose(std::exchange(f, nullptr)) == EOF) {
|
||||
throw std::ios_base::failure("fclose() failed");
|
||||
}
|
||||
}
|
||||
// Allow access to underlying file via `f`.
|
||||
FILE *handle() { return f; }
|
||||
// Release `f`, i.e., stop managing it.
|
||||
FILE *release() { return std::exchange(f, nullptr); }
|
||||
};
|
||||
|
||||
void file_user() {
|
||||
File fh{"example.txt", "r"};
|
||||
FILE *f = fh.handle();
|
||||
// Use `f` for the desired file operation(s).
|
||||
//
|
||||
// The file stream managed by `fh` will be automatically closed when `fh` goes
|
||||
// out of scope at the end of this function.
|
||||
}
|
||||
----
|
||||
Loading…
x
Reference in New Issue
Block a user