Fix another hang under ASAN

We have a long-running program that we want to run under sanitizers for
extra error checking and under faketime to speed up the clock. This
program hangs after a while. Backtraces suggest that the hangs occur
because of recursion in the memory allocator, which apparently locks a
non-recursive mutex.

Specifically, what we see is that due to our use of FAKETIME_NO_CACHE=1,
libfaketime wants to reload the config file inside a (random) call to
clock_gettime(). libfaketime then uses fopen() / fclose() for reading
the config files. These function allocate / free a buffer for reading
data and specifically the call to free() that happens inside fclose()
ends up calling clock_gettime() again. At this point, libfaketime locks
up because it has a time_mutex that is locked and none-recursive.

Sadly, I did not manage to come up with a stand-alone reproducer for
this problem. Also, the above description is from memory after half a
week of vacation, so it might be (partially) wrong.

More information can be found here:

- https://github.com/wolfcw/libfaketime/issues/365#issuecomment-1115802530
- https://github.com/wolfcw/libfaketime/issues/365#issuecomment-1116178907

This commit first adds a test case. This new test uses the already
existing libmallocintercept.so to cause calls to clock_gettime() during
memory allocation routines. The difference to the already existing
version is that additionally FAKETIME_NO_CACHE and
FAKETIME_TIMESTAMP_FILE are set. This new test hung with its last output
suggesting a recursive call to malloc:

Called malloc() from libmallocintercept...successfully
Called free() on from libmallocintercept...successfully
Called malloc() from libmallocintercept...Called malloc() from libmallocintercept...

Sadly, gdb was unable to provide a meaningful backtrace for this hang.

Next, I replaced the use of fopen()/fgets()/fgets() with
open()/read()/close(). This code no longer reads the config file
line-by-line, but instead it reads all of it at once and then "filters
out" the result (ignores comment lines, removes end of line markers).

I tried to keep the behaviour of the code the same, but I know at least
one difference: Previously, the config file was read line-by-line and
lines that began with a comment character were immediately ignored. The
new code only reads the config once and then removes comment lines.
Since the buffer that is used contains only 256 characters, it is
possible that config files that were previously parsed correctly now
longer parse. A specific example: if a file begins with 500 '#'
characters in its first line and then a timestamp in the second line,
the old code was able to parse this file while the new code would only
see an empty file.

After this change, the new test no longer hangs. Sadly, I do not
actually know its effect on the "actual bug" that I wanted to fix, but
since there are no longer any calls to fclose(), there cannot be any
hangs inside fclose().

Signed-off-by: Uli Schlachter <psychon@znc.in>

2 files changed, 53 insertions, 28 deletions

diff --git a/src/libfaketime.c b/src/libfaketime.c
index ec80ec8..2f26bd4 100644
--- a/src/libfaketime.c
+++ b/src/libfaketime.c
@@ -2968,20 +2968,40 @@ static void ftpl_init(void)
  *      =======================================================================
  */
 
-static void remove_trailing_eols(char *line)
+static void prepare_config_contents(char *contents)
 {
-  char *endp = line + strlen(line);
-  /*
-   * erase the last char if it's a newline
-   * or carriage return, and back up.
-   * keep doing this, but don't back up
-   * past the beginning of the string.
+  /* This function
+   * - removes line separators (\r and \n)
+   * - removes lines beginning with a comment character (# or ;)
    */
-# define is_eolchar(c) ((c) == '\n' || (c) == '\r')
-  while (endp > line && is_eolchar(endp[-1]))
-  {
-    *--endp = '\0';
+  char *read_position = contents;
+  char *write_position = contents;
+  bool in_comment = false;
+  bool beginning_of_line = true;
+
+  while (*read_position != '\0') {
+    if (beginning_of_line && (*read_position == '#' || *read_position == ';')) {
+      /* The line begins with a comment character and should be completely ignored */
+      in_comment = true;
+    }
+    beginning_of_line = false;
+
+    if (*read_position == '\n') {
+      /* We reached the end of the line that should be ignored (if any is ignored) */
+      in_comment = false;
+      /* The next character begins a new line */
+      beginning_of_line = true;
+    }
+
+    /* If we are not in a comment and are not looking at a line break, copy the
+     * character from the read position to the write position. */
+    if (!in_comment && *read_position != '\r' && *write_position != '\n') {
+      *write_position = *read_position;
+      write_position++;
+    }
+    read_position++;
   }
+  *write_position = '\0';
 }
 
 
@@ -3018,30 +3038,25 @@ int read_config_file()
   static char user_faked_time[BUFFERLEN]; /* changed to static for caching in v0.6 */
   static char custom_filename[BUFSIZ];
   static char filename[BUFSIZ];
-  FILE *faketimerc;
+  int faketimerc;
   /* check whether there's a .faketimerc in the user's home directory, or
    * a system-wide /etc/faketimerc present.
    * The /etc/faketimerc handling has been contributed by David Burley,
    * Jacob Moorman, and Wayne Davison of SourceForge, Inc. in version 0.6 */
   (void) snprintf(custom_filename, BUFSIZ, "%s", getenv("FAKETIME_TIMESTAMP_FILE"));
   (void) snprintf(filename, BUFSIZ, "%s/.faketimerc", getenv("HOME"));
-  if ((faketimerc = fopen(custom_filename, "rt")) != NULL ||
-      (faketimerc = fopen(filename, "rt")) != NULL ||
-      (faketimerc = fopen("/etc/faketimerc", "rt")) != NULL)
-  {
-    static char line[BUFFERLEN];
-    while (fgets(line, BUFFERLEN, faketimerc) != NULL)
-    {
-      if ((strlen(line) > 1) && (line[0] != ' ') &&
-          (line[0] != '#') && (line[0] != ';'))
-      {
-        remove_trailing_eols(line);
-        strncpy(user_faked_time, line, BUFFERLEN-1);
-        user_faked_time[BUFFERLEN-1] = 0;
-        break;
-      }
+  if ((faketimerc = open(custom_filename, O_RDONLY)) != -1 ||
+      (faketimerc = open(filename, O_RDONLY)) != -1 ||
+      (faketimerc = open("/etc/faketimerc", O_RDONLY)) != -1)
+  {
+    ssize_t length = read(faketimerc, user_faked_time, sizeof(user_faked_time) - 1);
+    close(faketimerc);
+    if (length < 0) {
+      length = 0;
     }
-    fclose(faketimerc);
+    user_faked_time[length] = 0;
+
+    prepare_config_contents(user_faked_time);
     parse_ft_string(user_faked_time);
     return 1;
   }
diff --git a/test/test.sh b/test/test.sh
index 1ef7439..b03077d 100755
--- a/test/test.sh
+++ b/test/test.sh
@@ -72,6 +72,16 @@ LD_PRELOAD="./libmallocintercept.so:$FTPL" ./timetest
 echo
 
 echo "============================================================================="
+echo
+
+echo "@2005-03-29 14:14:14" > .faketimerc-for-test
+echo "Running the test program with malloc interception and file faketimerc"
+echo "\$ FAKETIME_NO_CACHE=1 FAKETIME_TIMESTAMP_FILE=.faketimerc-for-test LD_PRELOAD=./libmallocintercept.so:$FTPL ./timetest"
+FAKETIME_NO_CACHE=1 FAKETIME_TIMESTAMP_FILE=.faketimerc-for-test LD_PRELOAD="./libmallocintercept.so:$FTPL" ./timetest
+rm .faketimerc-for-test
+echo
+
+echo "============================================================================="
 echo "Testing finished."
 
 exit 0