Set mode 0600 for log files and allow some configuration of logging

[Arfrever]

Creating log files readable for other users is insecure in case of LibRIME library, since all text entered when RIME is enabled can be found in these log files. I suggest to set mode 0600. Glog library respects FLAGS_logfile_mode global variable.

Log files for INFO log level can be huge. Short usage of Fcitx-RIME creates rime.fcitx-rime.….log.INFO.… with size about 200 MiB and rime.fcitx-rime.….log.WARNING.… with size about 1.5 MiB. I suggest that LibRIME library expose ability to set minimal log level which could be used by consumers of LibRIME library (e.g. Fcitx-RIME, IBus-RIME). Glog library respects FLAGS_minloglevel global variable.

Ability to use custom log directory may be also useful. Glog library respects FLAGS_log_dir global variable.

Patch: librime-logging_configuration.patch.txt

Gentoo bug: https://bugs.gentoo.org/695702

[lotem]

Snippets included in the Gentoo bug report are debug-level logs. Debug-delvel logs should be turned off in release build. Clearly it isn't the case. Logging options in the librime build could be misconfigured either by fcitx-rime (owned by Fcitx team) or in Gentoo ebuild.

[Arfrever]

ENABLE_LOGGING option is enabled by default in CMakeLists.txt:

librime/CMakeLists.txt

Line 19 in 6662a28

option(ENABLE_LOGGING "Enable logging with google-glog library" ON)

librime ebuild in Gentoo does not pass -DENABLE_LOGGING=ON or -DENABLE_LOGGING=OFF option to cmake:
https://gitweb.gentoo.org/repo/gentoo.git/tree/app-i18n/librime/librime-1.5.3.ebuild?id=47186617067461a05d4f28dc1a26f5a254bfb7dd#n55

If you consider logging a debugging feature and recommend users to disable it, you should change default value:

--- CMakeLists.txt
+++ CMakeLists.txt
@@ -16,7 +16,7 @@
 option(BUILD_SAMPLE "Build sample Rime plugin" OFF)
 option(BUILD_TEST "Build and run tests" ON)
 option(BUILD_SEPARATE_LIBS "Build separate rime-* libraries" OFF)
-option(ENABLE_LOGGING "Enable logging with google-glog library" ON)
+option(ENABLE_LOGGING "Enable logging with google-glog library" OFF)
 option(BOOST_USE_CXX11 "Boost has been built with C++11 support" OFF)
 option(BOOST_USE_SIGNALS2 "Boost use signals2 instead of signals" ON)
 option(ENABLE_ASAN "Enable Address Sanitizer (Unix Only)" OFF)

But even then, to have reasonable behavior for users explicitly using -DENABLE_LOGGING=ON, LibRIME should at least set reasonable permissions for log files:

--- src/rime/setup.cc
+++ src/rime/setup.cc
@@ -35,6 +35,8 @@
 
 RIME_API void SetupLogging(const char* app_name) {
 #ifdef RIME_ENABLE_LOGGING
+  // Do not allow other users to read/write log files created by current process.
+  FLAGS_logfile_mode = 0600;
   google::InitGoogleLogging(app_name);
 #endif  // RIME_ENABLE_LOGGING
 }

Neither Fcitx-RIME nor IBus-RIME directly call any Glog functions or set any Glog variables.
I think that providing ability of customization through LibRIME API (e.g. in RimeTraits structure as proposed in full patch) would be better design than having to set Glog variables directly in Fcitx-RIME or IBus-RIME.
If ability of customization is accepted in LibRIME API, I can contact Fcitx-RIME and IBus-RIME projects with suggestion to use it.

[lotem]

ENABLE_LOGGING=ON is by design. LOG(level) messages help users to locate errors often caused by misconfiguration.
However, the expected default behaviour, and current behaviour observed on other platforms is to silent all debug-mode logs (DLOG(level)) in release build, irrepective of log level.
I haven't got time to look into how this is not working as expected in Gentoo release. It could be affected by code/build settings other than librime. That's the real offending issue.

Taken separately, setting more restrictive file mode is a wanted improvement, and thank you very much for the patch. I need more time to make sure this change works for all platforms/building options, including the case where the optional gflags library is not used.
Please be aware that fixing file permission doens't resolve resource and privacy issues caused by excessive debug logs.

[Arfrever]

Apparently behavior of DLOG and other D* macros is dependent on NDEBUG and DCHECK_ALWAYS_ON:
https://github.com/google/glog/blob/4db06313464dadb5c5ca2619f3a461f0b61a07dc/src/glog/logging.h.in#L434-L438
https://github.com/google/glog/blob/4db06313464dadb5c5ca2619f3a461f0b61a07dc/src/glog/logging.h.in#L981-L1094

NDEBUG would be enabled by -DNDEBUG compiler flag. This flag is not used by default in Gentoo, but Gentoo users can set custom CFLAGS / CXXFLAGS / LDFLAGS etc. and some users may have added -DNDEBUG to their flags.

We can change librime ebuild in Gentoo to add -DNDEBUG to CXXFLAGS by default and add debug USE flag to optionally enable debug logging. debug USE flag would be disabled by default.

[Arfrever]

What do you think about adding an option to CMakeLists.txt?:

--- CMakeLists.txt
+++ CMakeLists.txt
@@ -18,4 +18,5 @@
 option(BUILD_SEPARATE_LIBS "Build separate rime-* libraries" OFF)
 option(ENABLE_LOGGING "Enable logging with google-glog library" ON)
+option(ENABLE_DEBUG "Enable debug mode (mostly affecting logging)" OFF)
 option(BOOST_USE_CXX11 "Boost has been built with C++11 support" OFF)
 option(BOOST_USE_SIGNALS2 "Boost use signals2 instead of signals" ON)
@@ -91,4 +92,12 @@
 endif()
 
+if(ENABLE_DEBUG)
+  if(ENABLE_LOGGING)
+    add_definitions(-DDCHECK_ALWAYS_ON)
+  endif()
+else()
+  add_definitions(-DNDEBUG)
+endif()
+
 find_package(Threads)
 

[lotem]

Nice findings. I agree that by default the Gentoo package should be built with debug features off.
Would it be enough to just use the cmake command line flag as below, if user do want debug build with excessive logs?
cmake -DCMAKE_BUILD_TYPE=Debug

[Arfrever]

Gentoo uses -DCMAKE_BUILD_TYPE=Gentoo by default, so I will not change this value to -DCMAKE_BUILD_TYPE=Debug or something else.

Suggested ENABLE_DEBUG option in LibRIME's CMakeLists.txt is not required to improve -DDCHECK_ALWAYS_ON / -DNDEBUG usage in LibRIME in Gentoo, but it could be helpful for other users (outside of Gentoo) who build LibRIME and may be affected by big logs with debugging messages.

More importantly, could you accept the change in src/rime/setup.cc?

[Arfrever]

Ping.
Can you at least apply change proposed above for src/rime/setup.cc?

If you dislike change proposed for CMakeLists.txt then I can implement manual handling of -DNDEBUG / -DDCHECK_ALWAYS_ON flags...

[lotem]

Sorry for the late reply.
I haven't done the verification I mentioned in ealier reply #316 (comment)
since I failed my struggle setting up a Gentoo system for figuring out the situation you reported.

I'll simply skip that issue and test your file-mode patch in Windows, macOS and any Linux distro at hand, so make sure it doesn't break anything before merging.

[Arfrever]

You do not actually need to have Gentoo for testing.
CMAKE_BUILD_TYPE can have any value, it is not restricted to known-to-CMake values like Debug and Release.
Gentoo ebuilds use -DCMAKE_BUILD_TYPE=Gentoo by default, so that user-requested CFLAGS / CXXFLAGS are more likely to be respected.

If cmake is called for librime with -DCMAKE_BUILD_TYPE=something_not_standard option, then src/CMakeFiles/rime.dir/flags.make contains:

CXX_FLAGS =  -std=c++11 -fPIC

If cmake is called for librime with -DCMAKE_BUILD_TYPE=Debug option, then src/CMakeFiles/rime.dir/flags.make contains:

CXX_FLAGS =  -std=c++11 -g -fPIC

If cmake is called for librime with -DCMAKE_BUILD_TYPE=Release option, then src/CMakeFiles/rime.dir/flags.make contains:

CXX_FLAGS =  -std=c++11 -O3 -DNDEBUG -fPIC

[lotem]

There is a blocking build failure in PR #328 due to using old version of gflags library in ubuntu trusty. I'm going to try upgrading the travis image first.

[Arfrever]

logfile_mode is a feature of Glog library, not Gflags library.
Apparently it was added in this commit and released in Glog 0.3.5 (released on 2017-05-10).

I suspect that you might want to support older versions of Glog library, in which case probably something like the following can help:

--- CMakeLists.txt
+++ CMakeLists.txt
@@ -85,6 +85,9 @@
   if(Glog_STATIC)
     add_definitions(-DGOOGLE_GLOG_DLL_DECL=)
   endif()
+  if(Glog_VERSION VERSION_GREATER_EQUAL 0.3.5)
+    add_definitions(-DRIME_GLOG_0_3_5)
+  endif()
 
   add_definitions(-DRIME_ENABLE_LOGGING)
 
--- src/rime/setup.cc
+++ src/rime/setup.cc
@@ -39,8 +39,10 @@
   if (log_dir) {
     FLAGS_log_dir = log_dir;
   }
+#ifdef RIME_GLOG_0_3_5
   // Do not allow other users to read/write log files created by current process.
   FLAGS_logfile_mode = 0600;
+#endif
   google::InitGoogleLogging(app_name);
 #endif  // RIME_ENABLE_LOGGING
 }