Advice needed re: threadsafe ctime (and friends)

[chennes]

The top C++ issues that LGTM is complaining about right now are two calls to old-school C time functions: ctime and localtime -- these functions use a global static time structure, and so aren't thread safe. I'd looked at this briefly last year and decided that the answer was sufficiently complex that it needed to be delayed until after 0.19 was final. So, here we are now, and I've fallen down the rabbit hole again.

For the record, this problem occurs in exactly two places in FreeCAD: creating timestamps for console output, and creating timestamps for backup file filename generation. So the reality is that this "problem" is totally trivial and could of course be ignored entirely. But the universe has waved this problem in front of my face, and now I must solve it (I hope some here can sympathize!).

Here are the solutions I've tried, in order:

Option 1: ctime_r and localtime_r. These are non-portable, and aren't actually in the C++ standard. To make matters worse, Microsoft doesn't use the same parameter order that GNU C does, so using them is a complete mess.

Option 2: Boost date_time. It turns out that under the hood, Boost's timezone conversion uses the old C API, so it's not actually threadsafe. It would hide the LGTM warning I think, but wouldn't actually solve the underlying problem.

Option 3: Unicode ICU Date/Time. This would actually be nice, except it's only been included in Windows since Windows 10 v1703. I don't know how feasible it is for us to use (it looks like it's getting packaged, but I don't see any references to it being used in FreeCAD).

Option 4: Qt QDateTime and QTimezone. Perfect solution. Except they don't use the standard strftime format strings, so either we invalidate anyone's current custom backup file timestamp name scheme, or we have to write a translator (which will surely break someone's custom scheme).

I am open to ideas, thoughts, and commiseration. I had never given much thought to the complexity of timezones. Wow.

[openBrain]

Sorry if it's silly thinking but I see no real problem behind the "it's not thread safe". What does it functionally imply? Some milliseconds error in the date?

Anyway, could it also be a possibility to use Python time functions?

[saso]

LGTM recommends to replace it with ctime_r https://lgtm.com/rules/2154840805/ (Use of potentially dangerous function) while the referenced link from that page to https://wiki.sei.cmu.edu/confluence/dis ... +functions (SEI CERT C Coding Standard) recommends to use the ctime_s

As I can see from the report there are only three issues for this in Application.cpp, Document.cpp and zipoutputstreambuf.cpp and they are all marked as warnings, so maybe not even that critical to fix it

[chennes]

Sorry if it's silly thinking but I see no real problem behind the "it's not thread safe". What does it functionally imply?

In this particular application, you're not wrong -- I doubt this particular data race has ever affected anyone using FreeCAD. It is easy enough to replace the Console time output function with a threadsafe variant, leaving only the backup file creation routine trying to access the library-allocated struct tm, and that code should never be called by multiple threads. So this is really a sort of "academic" bug -- it's just grating on me that I can't come up with a straightforward solution.

LGTM recommends to replace it with ctime_r https://lgtm.com/rules/2154840805/ (Use of potentially dangerous function) while the referenced link from that page to https://wiki.sei.cmu.edu/confluence/dis ... +functions (SEI CERT C Coding Standard) recommends to use the ctime_s

This suggested fix from LGTM is a bad recommendation on their part, I think -- the ctime_s and ctime_r functions are not portable. You are completely right that it's not critical to fix it -- I just fell down the rabbit hole of what turned out to be a much more interesting (and frustrating!) problem than expected, and was interested in other developers' ideas on solutions.

[wmayer]

The function Application::logStatus() uses

Code: Select all

    time_t now;
    time(&now);
    ctime(&now);

That produces e.g.: Thu Sep 30 19:10:06 2021

Code: Select all

    boost::posix_time::to_simple_string(
        boost::posix_time::second_clock::local_time());

and boost::posix_time::to_simple_string produces: 2021-Sep-30 19:10:06

In this case the boost function is an acceptable replacement

git commit 0c62c22554

[chennes]

I think that will silence LGTM, but if we are actually concerned about the thread safety, it turns out that Boost is just calling ctime() and friends under the hood.

[wmayer]

I think that will silence LGTM, but if we are actually concerned about the thread safety, it turns out that Boost is just calling ctime() and friends under the hood.

In none of the functions thread-safety is an issue.