File descriptor closed twice

[sobkulir]

F´ Version	3.3.2
Affected Component	Os/FileManager

---

Problem Description

When calling Os::FileSystem::copyFile() and Os::FileSystem::appendFile(), the source and destination file descriptors (FD) are attempted to be closed twice. This should be logged somewhere as error, but the return value of closing the FD is ignored in the implementation of Os::File::close().

The root cause is described below.

In Os/Linux/File.cpp, there's a File::close() call from the destructor:

File::~File() {
    if (this->m_mode != OPEN_NO_MODE) {
        this->close();
    }
}

The Os::FileSystem::copyFile() and Os::FileSystem::appendFile() call helper function copyFileData() which takes source and destination files as value arguments. Therefore, when the function returns, the files are closed because of the destructor, while at the same time, they are closed again in the calling function.

Status copyFileData(File source, File destination, FwSizeType size) {
   // Copying logic...

   // At the end, `source` and `destination` destructor closes the files.
}

Status copyFile(const char* originPath, const char* destPath) {
    File source;
    File destination;
    File::Status file_status;

    fs_status = copyFileData(source, destination, fileSize);
   
    // Files are closed again because the `source.m_mode` and `destination.m_mode` are
    // not set to OPEN_NO_MODE in these instances. 
    (void)source.close();
    (void)destination.close();

    return fs_status;
}

In the actual Os::File::close() implementation, the return value of operating system's close(FD) is ignored, I presume in order to be able to call the routine in the destructor.

void File::close() {
    if ((this->m_fd != -1) and (this->m_mode != OPEN_NO_MODE)) {
        (void)::close(this->m_fd);
        this->m_fd = -1;
    }
    this->m_mode = OPEN_NO_MODE;
}

How to Reproduce

For the Posix implementation, this doesn't lead to any big trouble, however, I ran into issues when implementing a port for Zephyr. I believe a nicer interface would be to either assert in the destructor that the file is closed and pass it always by reference, or make the destructor a noop.

Expected Behavior

The files should only be closed once.

[vietjtnguyen]

I think a good fix is to delete the copy constructor for the Os::File object since this is a RIAA resource owning object and the underlying resource can't be copied. For cases where you have multiple readers/writers to the file a "weak handle" object (Os::WeakFile?) could be introduced to have a non-owning reference to the underlying resource.

[LeStarch]

In the Os::File refactor, we implemented a working copy constructor which duplicates the file object. This will fix this problem. In an upcoming PR we will refactor the FileSystem and ensure these are passed by reference for efficiency.

[LeStarch]

Done and delivered. Let me know if more issues arise here.