LGTM.com - false positive in C++ code #ifdef'd out

[jimis]

I got the alert "Call to localtime is potentially dangerous", but the code annotated is inside an ifdef, and it shouldn't be included on the usual platforms.

The code in a nutshell:

#if defined(_POSIX_THREAD_SAFE_FUNCTIONS)
    localtime_r();
#else
    localtime();
    // LGTM.COM WARNING: Call to localtime is potentially dangerous
#endif

I have verified that configuring and compiling on Ubuntu Linux calls the re-entrant function. The mere existence of the non-reentrant fallback shouldn't cause a warning.

Link to the original warning

[jbj]

Thank you for the report. The way our analysis works, it should only observe code that's compiled. Since you're getting this alert, it means our tooling is picking the wrong branch in the #if. If GCC compiles one version of your code, and the CodeQL extractor compiles another, then we have a bug.

I'm not familiar with POSIX options, so I tried to dig up the documentation. According to sysconf(3), preprocessor macros like _POSIX_THREAD_SAFE_FUNCTIONS are exposed by including unistd.h, but the include of unistd.h in the file with the alert is conditional:

https://github.com/qt/qtbase/blob/76c3eee4020cae681857ee17406c655f61f9082c/src/corelib/time/qdatetime.cpp#L60-L62

Why is unistd.h only included on MinGW platforms? The code comment states clearly that the purpose of the include is to expose _POSIX_THREAD_SAFE_FUNCTIONS, so shouldn't that file be included on all Unix platforms?

[jimis]

unistd.h should be included by qplatformdefs.h. On my ubuntu system I preprocessed the file with both unistd.h explicitly included on top of qdatetime.cpp, and as is now. In both cases localtime_r() was invoked, so I'm assuming unistd.h is included.

So I think that because of all the LGTM.com tooling, the wrong qplatformdefs.h is used. I wish I could do small changes on my private fork and test LGTM.com results, but that is not possible AFAIK, right?

[jimis]

By checking QT_PLATFORM_DEFINITION_DIR in CMakeCache.txt in the build directory we can find out which qplatformdefs.h is included.

[jbj]

You're right that unistd.h is included by qplatformdefs.h: clicking the link on https://lgtm.com/projects/g/qt/qtbase/snapshot/8277a49f672cea416d7988bc35a4bec494359982/files/src/corelib/time/qdatetime.cpp?sort=name&dir=ASC&mode=heatmap#L41 takes us to the version of qplatformdefs.h that was used for the compilation on LGTM.

[jbj]

@igfoo, can you or someone else from the extractor team take over this issue? It looks like our extractor takes a different path through an #if than GCC does.

[jimis]

clicking the link on https://lgtm.com/projects/g/qt/qtbase/snapshot/8277a49f672cea416d7988bc35a4bec494359982/files/src/corelib/time/qdatetime.cpp?sort=name&dir=ASC&mode=heatmap#L41 takes us to the version of qplatformdefs.h that was used for the compilation on LGTM.

Nice, I did not notice this was click-able! Good to know, will definitely help in the future.

[igfoo]

I'll take a look.

[igfoo]

It looks like we're making the same choice as g++. I added

#if QT_CONFIG(thread)
#warning Y1
#endif
#if defined(_POSIX_THREAD_SAFE_FUNCTIONS)
#warning Y2
#endif

#if QT_CONFIG(thread) && defined(_POSIX_THREAD_SAFE_FUNCTIONS)
#warning X1
#elif defined(Q_CC_MSVC)
#warning X2
#else
#warning X3
#endif

to the file and got this output:

$ g++ -DHAVE_CONFIG_H -DPCRE2_CODE_UNIT_WIDTH=16 -DPCRE2_DISABLE_JIT -DQT_BOOTSTRAPPED -DQT_NO_CAST_FROM_ASCII -DQT_NO_CAST_TO_ASCII -DQT_NO_FOREACH -DQT_USE_QSTRINGBUILDER -DQT_VERSION_MAJOR=6 -DQT_VERSION_MINOR=0 -DQT_VERSION_PATCH=0 '-DQT_VERSION_STR="6.0.0"' -Isrc/tools/bootstrap/../../3rdparty/pcre2/src -Isrc/corelib/Core_autogen/include -Iinclude -Iinclude/QtCore -Isrc/corelib -Isrc/corelib/global -Isrc/corelib/../3rdparty/tinycbor/src -Iinclude/QtCore/6.0.0 -Iinclude/QtCore/6.0.0/QtCore -Isrc/corelib/../3rdparty/forkfd -Isrc/corelib/.rcc '-Imkspecs/linux-g++' -I/usr/include/double-conversion -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -Isrc/xml/Xml_autogen/include -Iinclude/QtXml -Isrc/xml -Iinclude/QtXml/6.0.0 -Iinclude/QtXml/6.0.0/QtXml -Isrc/tools/bootstrap/.. -Isrc/tools/bootstrap/../../3rdparty/tinycbor/src -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Wall -Wextra -ffunction-sections -fdata-sections -mshstk '-std=gnu++17' -MD -MT src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/time/qdatetime.cpp.o -MF src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/time/qdatetime.cpp.o.d -o src/tools/bootstrap/CMakeFiles/Bootstrap.dir/__/__/corelib/time/qdatetime.cpp.o -c src/corelib/time/qdatetime.cpp
src/corelib/time/qdatetime.cpp:2480:2: warning: #warning Y2 [-Wcpp]
 2480 | #warning Y2
      |  ^~~~~~~
src/corelib/time/qdatetime.cpp:2488:2: warning: #warning X3 [-Wcpp]
 2488 | #warning X3
      |  ^~~~~~~

[jimis]

I see, it seems that QT_FEATURE_thread is OFF in your CMakeCache.txt. Can you verify that? FWIW it is on for me:

$ grep QT_FEATURE_thread: CMakeCache.txt 
QT_FEATURE_thread:INTERNAL=ON

[jimis]

Checking the configure log from LGTM.com, I see that threads are detected:

[2020-06-29 22:03:38] [build] -- Looking for pthread.h
[2020-06-29 22:03:38] [build] -- Looking for pthread.h - found
[2020-06-29 22:03:38] [build] -- Performing Test CMAKE_HAVE_LIBC_PTHREAD
[2020-06-29 22:03:38] [build] -- Performing Test CMAKE_HAVE_LIBC_PTHREAD - Failed
[2020-06-29 22:03:38] [build] -- Looking for pthread_create in pthreads
[2020-06-29 22:03:39] [build] -- Looking for pthread_create in pthreads - not found
[2020-06-29 22:03:39] [build] -- Looking for pthread_create in pthread
[2020-06-29 22:03:39] [build] -- Looking for pthread_create in pthread - found
[2020-06-29 22:03:39] [build] -- Found Threads: TRUE  

I get the exact same output on my system.