Merge pull request #160 from MrAnno/fix-wildcard-delete-race

wildcard-file: fix race between EOF and file deletion detection

modules/affile/poll-file-changes.c

@@ -1,6 +1,8 @@
 /*
  * Copyright (c) 2002-2013 Balabit
+ * Copyright (c) 2024 Axoflow
  * Copyright (c) 1998-2013 Balázs Scheidler
+ * Copyright (c) 2024 László Várady
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published
@@ -61,6 +63,10 @@ poll_file_changes_on_eof(PollFileChanges *self)
   if (self->on_eof)
     result = self->on_eof(self);
   log_pipe_notify(self->control, NC_FILE_EOF, self);
+
+  if (self->stop_on_eof)
+    return FALSE;
+
   return result;
 }
 
@@ -214,6 +220,14 @@ poll_file_changes_update_watches(PollEvents *s, GIOCondition cond)
     poll_file_changes_rearm_timer(self);
 }
 
+void
+poll_file_changes_stop_on_eof(PollEvents *s)
+{
+  PollFileChanges *self = (PollFileChanges *) s;
+
+  self->stop_on_eof = TRUE;
+}
+
 void
 poll_file_changes_free(PollEvents *s)
 {

modules/affile/poll-file-changes.h

@@ -1,6 +1,8 @@
 /*
  * Copyright (c) 2002-2013 Balabit
+ * Copyright (c) 2024 Axoflow
  * Copyright (c) 1998-2013 Balázs Scheidler
+ * Copyright (c) 2024 László Várady
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published
@@ -39,6 +41,7 @@ struct _PollFileChanges
   struct iv_timer follow_timer;
   LogPipe *control;
 
+  gboolean stop_on_eof;
   void (*on_read)(PollFileChanges *);
   gboolean (*on_eof)(PollFileChanges *);
   void (*on_file_moved)(PollFileChanges *);
@@ -50,6 +53,7 @@ void poll_file_changes_init_instance(PollFileChanges *self, gint fd, const gchar
                                      LogPipe *control);
 void poll_file_changes_update_watches(PollEvents *s, GIOCondition cond);
 void poll_file_changes_stop_watches(PollEvents *s);
+void poll_file_changes_stop_on_eof(PollEvents *s);
 void poll_file_changes_free(PollEvents *s);
 
 #endif

modules/affile/tests/Makefile.am

@@ -29,8 +29,9 @@ modules_affile_tests_test_file_opener_LDADD	= $(TEST_LDADD) \
 	-dlpreopen $(top_builddir)/modules/affile/libaffile.la
 
 modules_affile_tests_test_wildcard_file_reader_CFLAGS = $(TEST_CFLAGS) -I$(top_srcdir)/modules/affile
-modules_affile_tests_test_wildcard_file_reader_LDADD	= $(TEST_LDADD)
-modules_affile_tests_test_wildcard_file_reader_SOURCES = modules/affile/tests/test_wildcard_file_reader.c modules/affile/wildcard-file-reader.c
+modules_affile_tests_test_wildcard_file_reader_LDADD	= $(TEST_LDADD) \
+	-dlpreopen $(top_builddir)/modules/affile/libaffile.la
+modules_affile_tests_test_wildcard_file_reader_SOURCES = modules/affile/tests/test_wildcard_file_reader.c
 
 modules_affile_tests_test_file_list_CFLAGS = $(TEST_CFLAGS) -I$(top_srcdir)/modules/affile
 modules_affile_tests_test_file_list_LDADD	= $(TEST_LDADD) \

modules/affile/tests/test_wildcard_file_reader.c

@@ -1,5 +1,7 @@
 /*
  * Copyright (c) 2018 Balabit
+ * Copyright (c) 2024 Axoflow
+ * Copyright (c) 2024 László Várady
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published
@@ -28,14 +30,14 @@
 #include "iv.h"
 #include <glib/gstdio.h>
 #include <unistd.h>
+#include "poll-file-changes.h"
 
 #define TEST_FILE_NAME "TEST_FILE"
 
 
 typedef struct _TestFileStateEvent
 {
   gboolean deleted_eof_called;
-  gboolean finished_called;
 } TestFileStateEvent;
 
 static void
@@ -97,6 +99,8 @@ void file_reader_init_instance(FileReader *self, const gchar *filename,
                                LogSrcDriver *owner, GlobalConfig *cfg)
 {
   log_pipe_init_instance(&self->super, cfg);
+  self->reader = log_reader_new(cfg);
+  self->reader->poll_events = poll_file_changes_new(-1, "", 1, &self->super);
   return;
 }
 
@@ -124,17 +128,10 @@ TestSuite(test_wildcard_file_reader, .init = _init, .fini = _teardown);
 
 Test(test_wildcard_file_reader, constructor)
 {
-  cr_assert_eq(reader->file_state.eof, FALSE);
+  cr_assert_eq(reader->file_state.last_eof, FALSE);
   cr_assert_eq(reader->file_state.deleted, FALSE);
 }
 
-Test(test_wildcard_file_reader, msg_read)
-{
-  reader->file_state.eof = TRUE;
-  log_pipe_queue(&reader->super.super, NULL, &path_options);
-  cr_assert_eq(reader->file_state.eof, FALSE);
-}
-
 Test(test_wildcard_file_reader, notif_deleted)
 {
   log_pipe_queue(&reader->super.super, NULL, &path_options);
@@ -143,11 +140,11 @@ Test(test_wildcard_file_reader, notif_deleted)
 }
 
 
-Test(test_wildcard_file_reader, notif_eof)
+Test(test_wildcard_file_reader, eof_without_deletion_should_not_change_last_eof_state)
 {
   log_pipe_queue(&reader->super.super, NULL, &path_options);
   log_pipe_notify(&reader->super.super, NC_FILE_EOF, NULL);
-  cr_assert_eq(reader->file_state.eof, TRUE);
+  cr_assert_eq(reader->file_state.last_eof, FALSE);
 }
 
 Test(test_wildcard_file_reader, status_change_deleted_not_eof)
@@ -159,18 +156,13 @@ Test(test_wildcard_file_reader, status_change_deleted_not_eof)
 
 Test(test_wildcard_file_reader, status_change_deleted_eof)
 {
-  log_pipe_queue(&reader->super.super, NULL, &path_options);
-  log_pipe_notify(&reader->super.super, NC_FILE_DELETED, NULL);
-  log_pipe_notify(&reader->super.super, NC_FILE_EOF, NULL);
-  cr_assert_eq(test_event->deleted_eof_called, TRUE);
-}
-
-Test(test_wildcard_file_reader, status_finished_then_delete)
-{
-  log_pipe_queue(&reader->super.super, NULL, &path_options);
   log_pipe_notify(&reader->super.super, NC_FILE_EOF, NULL);
   cr_assert_eq(test_event->deleted_eof_called, FALSE);
 
   log_pipe_notify(&reader->super.super, NC_FILE_DELETED, NULL);
+  cr_assert_eq(test_event->deleted_eof_called, FALSE);
+
+  /* AxoSyslog waits for a last EOF check before deleting the file */
+  log_pipe_notify(&reader->super.super, NC_FILE_EOF, NULL);
   cr_assert_eq(test_event->deleted_eof_called, TRUE);
 }

modules/affile/wildcard-file-reader.c

@@ -1,5 +1,7 @@
 /*
  * Copyright (c) 2018 Balabit
+ * Copyright (c) 2024 Axoflow
+ * Copyright (c) 2024 László Várady
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published
@@ -22,13 +24,14 @@
 
 #include "wildcard-file-reader.h"
 #include "mainloop.h"
+#include "poll-file-changes.h"
 
 static gboolean
 _init(LogPipe *s)
 {
   WildcardFileReader *self = (WildcardFileReader *)s;
   self->file_state.deleted = FALSE;
-  self->file_state.eof = FALSE;
+  self->file_state.last_eof = FALSE;
   return file_reader_init_method(s);
 }
 
@@ -43,14 +46,6 @@ _deinit(LogPipe *s)
   return file_reader_deinit_method(s);
 }
 
-static void
-_queue(LogPipe *s, LogMessage *msg, const LogPathOptions *path_options)
-{
-  WildcardFileReader *self = (WildcardFileReader *)s;
-  self->file_state.eof = FALSE;
-  file_reader_queue_method(s, msg, path_options);
-}
-
 static void
 _deleted_file_eof(FileStateEvent *self, FileReader *reader)
 {
@@ -71,24 +66,32 @@ _schedule_state_change_handling(WildcardFileReader *self)
 }
 
 static void
-_set_eof(WildcardFileReader *self)
+_on_eof(WildcardFileReader *self)
 {
-  self->file_state.eof = TRUE;
   if (self->file_state.deleted)
     {
+      self->file_state.last_eof = TRUE;
       _schedule_state_change_handling(self);
     }
 }
 
 static void
-_set_deleted(WildcardFileReader *self)
+_on_deleted(WildcardFileReader *self)
 {
   /* File can be deleted only once,
    * so there is no need for checking the state
    * before we set it
    */
   self->file_state.deleted = TRUE;
-  _schedule_state_change_handling(self);
+
+  if (!self->super.reader || !self->super.reader->poll_events)
+    {
+      self->file_state.last_eof = TRUE;
+      _schedule_state_change_handling(self);
+      return;
+    }
+
+  poll_file_changes_stop_on_eof(self->super.reader->poll_events);
 }
 
 static void
@@ -98,10 +101,10 @@ _notify(LogPipe *s, gint notify_code, gpointer user_data)
   switch(notify_code)
     {
     case NC_FILE_DELETED:
-      _set_deleted(self);
+      _on_deleted(self);
       break;
     case NC_FILE_EOF:
-      _set_eof(self);
+      _on_eof(self);
       break;
     default:
       file_reader_notify_method(s, notify_code, user_data);
@@ -114,10 +117,10 @@ _handle_file_state_event(gpointer s)
 {
   WildcardFileReader *self = (WildcardFileReader *)s;
   msg_debug("File status changed",
-            evt_tag_int("EOF", self->file_state.eof),
+            evt_tag_int("EOF", self->file_state.last_eof),
             evt_tag_int("DELETED", self->file_state.deleted),
             evt_tag_str("Filename", self->super.filename->str));
-  if (self->file_state.deleted && self->file_state.eof)
+  if (self->file_state.deleted && self->file_state.last_eof)
     _deleted_file_eof(&self->file_state_event, &self->super);
 }
 
@@ -143,7 +146,6 @@ wildcard_file_reader_new(const gchar *filename, FileReaderOptions *options, File
   WildcardFileReader *self = g_new0(WildcardFileReader, 1);
   file_reader_init_instance(&self->super, filename, options, opener, owner, cfg);
   self->super.super.init = _init;
-  self->super.super.queue = _queue;
   self->super.super.notify = _notify;
   self->super.super.deinit = _deinit;
   IV_TASK_INIT(&self->file_state_event_handler);

modules/affile/wildcard-file-reader.h

@@ -1,5 +1,7 @@
 /*
  * Copyright (c) 2018 Balabit
+ * Copyright (c) 2024 Axoflow
+ * Copyright (c) 2024 László Várady
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License version 2 as published
@@ -40,7 +42,7 @@ typedef struct _FileStateEvent
 typedef struct _FileState
 {
   gboolean deleted;
-  gboolean eof;
+  gboolean last_eof;
 } FileState;
 
 

news/bugfix-160.md

@@ -0,0 +1 @@
+`wildcard-file()`: fix crash when a deleted file is concurrently written