Call to virtual method 'basic_pstreambuf::sync' during destruction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]

[JohanneMontano]

Hello,

I have written the wrapper function below

template <typename... CmdArgs>
inline std::pair<int, std::string> system_call(const std::string& cmd, CmdArgs&&... cmd_args) {
    auto cmd_str = fmt::format(fmt::runtime(cmd), std::forward<CmdArgs>(cmd_args)...);

    redi::ipstream pipe{};

    // NOLINTNEXTLINE(cert-env33-c) Try no linting to make clang tidy happy
    pipe.open(cmd_str, redi::pstreams::pstdout | redi::pstreams::pstderr);

    // Get the full output from our stdout pipe and put possible errors into its own buffer
    std::string result(std::istreambuf_iterator<char>(pipe.out()), std::istreambuf_iterator<char>());
    std::string possible_errors(std::istreambuf_iterator<char>(pipe.err()), std::istreambuf_iterator<char>());

    if (!possible_errors.empty()) {
        NUClear::log<NUClear::WARN>(
            fmt::format("Pipe pstderr non-empty while executing command {}: {}", cmd_str, possible_errors));
    }

    return {possible_errors.empty() ? 1 : 0, result};
}

calling the function yields the following clang tidy static analyser warning (as an error)

/usr/local/include/pstreams/pstream.h:1621:7: warning: Call to virtual method 'basic_pstreambuf::sync' during destruction bypasses virtual dispatch [clang-analyzer-optin.cplusplus.VirtualCall]
      sync(); // this might call wait() and reap the child process
      ^

/src/utilities.hpp:177:56: note: Calling '~basic_ipstream'
        return {possible_errors.empty() ? 1 : 0, result};
                                                       ^
/usr/local/include/pstreams/pstream.h:490:9: note: Calling '~pstream_common'
      { }
        ^
/usr/local/include/pstreams/pstream.h:2284:5: note: Calling '~basic_pstreambuf'
    }
    ^
/usr/local/include/pstreams/pstream.h:1202:7: note: Calling 'basic_pstreambuf::close'
      close();
      ^~~~~~~
/usr/local/include/pstreams/pstream.h:1621:7: note: Call to virtual method 'basic_pstreambuf::sync' during destruction bypasses virtual dispatch
      sync(); // this might call wait() and reap the child process

I made clang tidy happy by making pipe a static variable, which is not really ideal in my humble opinion. I have another bit of code that uses pstreams in a similar way but it is in a class as a private member variable (and I have not encountered this problem in that implementation) so my guess is that this problem is life time / scope related. Are there any proper fixes I can implement on my side?

Thank you very much in advance!!

[jwakely]

The code is correct, non-virtual dispatch is what's wanted there. But I'll add a workaround to stop the earning. Making it a qualified call will make it clear it's non-virtual:

basic_pstreambuf::sync();

[jwakely]

Isn't this #7 which was already fixed by 92e3207716804454690ab937c7bbbfb9e51c8b1c ?

It looks like you're using an old version of the code.

[JohanneMontano]

Thank you, I'll try and pull a new version of the code and will close this issue if that works.

[JohanneMontano]

I've just checked and I have pulled the code from Release version 1.0.3 in SourceForge. Is this fix in that version?

[jwakely]

No, you'll have to get the code from Git. You can just download the https://github.com/jwakely/pstreams/blob/master/pstream.h file.

I suppose I should do a 1.0.4 release...

[JohanneMontano]

Hi, it looks like https://github.com/jwakely/pstreams/archive/refs/tags/RELEASE_1_0_3.tar.gz contains the fix so I'll just pull from that. I was a bit confused since I assumed that the version for this release was similar to the latest SourceForge one, but this works perfectly for me.

[jwakely]

Yeah, it should be the same ... I'm a bit confused what's happened with the tags on github, they don't seem to match the releases. I'll look into it.

[jwakely]

I figured out what happened, it was messy.

I made a new release which should be consistent everywhere. Maybe I should just stop putting releases on sourceforge and then there's a single source of truth.

[JohanneMontano]

We have started pulling from the git releases so removing sf wont affect us but I think doing what's simplest and easiest for you is the best course of action :)) Thanks again!!!