Static analysis issues

[ghost]

Hello,
We are using spdlog (version 1.4.2) in one of the projects I am working on. Running Klocwork for static analysis against the project's code, several issues are reported in the library. Although many reported are false positives, some of them I believe are fixable. I have also checked and most reported issues are related to code that is not available in the newer version of the library (version 1.11.0).
Please check the list below.

1. void function returns value
   include/spdlog/fmt/bundled/format.h:2448 | operator()()
   Code: VOIDRET | Severity: Error (2) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

Fix:

...
if (!find<IS_CONSTEXPR>(from, to, Char('}'), p))
{
    handler_.on_text(from, to);
    return ;
}
++p;
...

2. Class specs_setter defines a copy constructor, but no assignment operator
   include/spdlog/fmt/bundled/format.h:1944 | format.h()
   Code: CWARN.COPY.NOASSIGN | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned
   Fix - removal:

FMT_CONSTEXPR specs_setter(const specs_setter& other)
    : specs_(other.specs_) {}

3. Class dynamic_specs_handler defines a copy constructor, but no assignment operator
   include/spdlog/fmt/bundled/format.h:2174 | format.h()
   Code: CWARN.COPY.NOASSIGN | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

Fix - removal:

  FMT_CONSTEXPR dynamic_specs_handler(const dynamic_specs_handler& other)
      : specs_setter<char_type>(other),
        specs_(other.specs_),
        context_(other.context_) {}

4. Class error_handler defines a copy constructor, but no assignment operator
   include/spdlog/fmt/bundled/core.h:407 | core.h()
   Code: CWARN.COPY.NOASSIGN | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

Fix - removal:

constexpr error_handler(const error_handler&) = default;

5. The following five more reported issues are related to the operations on the filenames.

321: function `rename` operates on file names and is vulnerable to race conditions. Files can be manipulated by attackers between creation and access time.
include/spdlog/sinks/rotating_file_sink-inl.h:126 | rotating_file_sink-inl.h()
Code: SV.TOCTOU.FILE_ACCESS | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

389: function `rename` operates on file names and is vulnerable to race conditions. Files can be manipulated by attackers between creation and access time.
include/spdlog/details/os-inl.h:186 | rename()
Code: SV.TOCTOU.FILE_ACCESS | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

420: function `open` operates on file names and is vulnerable to race conditions. Files can be manipulated by attackers between creation and access time.
include/spdlog/details/file_helper-inl.h:52 | reopen()
Code: SV.TOCTOU.FILE_ACCESS | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

439: function `fopen` operates on file names and is vulnerable to race conditions. Files can be manipulated by attackers between creation and access time.
include/spdlog/details/os-inl.h:155 | fopen_s()
Code: SV.TOCTOU.FILE_ACCESS | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

475: function `stat` operates on file names and is vulnerable to race conditions. Files can be manipulated by attackers between creation and access time.
include/spdlog/details/os-inl.h:202 | file_exists()
Code: SV.TOCTOU.FILE_ACCESS | Severity: Review (4) | State: Existing | Status: Analyze | Taxonomy: C and C++ | Owner: unowned

One of the possible solutions to these issues is replacing std::rename with renameat2.

// return std::rename(filename1.c_str(), filename2.c_str());
return renameat2(AT_FDCWD, filename1.c_str(), AT_FDCWD, filename2.c_str(), RENAME_NOREPLACE);

Please, share your opinion with me.

[tt4g]

Files under path include/spdlog/fmt/bundled are copied from the fmt library.
I recommend that you reopen the issue in fmt repository.

[ghost]

You're right. I have posted 4 issues related to fmt on their repository page.

[ghost]

Excuse me, do I understand well, that you don't plan to address 5 race condition issues in spdlog?

[tt4g]

spdlog only imports the latest version of fmt. spdlog will import new versions of fmt as soon as the bugs are fixed.

[tt4g]

Related fmtlib/fmt#3204

[ghost]

I have described 9 issues, only the first 4 relate to fmt. Last 5 address include/spdlog/details/os-inl.h file, is it also imported code?
I mention directly problem with this line.

[tt4g]

I had overlooked rotating_file_sink-inl.h, os-inl.h and file_helper-inl.h.
However, it is by design that parameters given externally to the reported API are passed to the corresponding API.
The reported problem cannot be fixed since the API is exposed for library consumers to specify the log file name.