From 4d200cd6938c1416e34bf43576b0d528b73e8ba3 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Thu, 17 Mar 2016 16:17:11 -0400 Subject: [PATCH 1/2] Fix a race in maintaining the journald reader list The journald log reader keeps a map of following readers so that it can close them properly when the journald reader object itself is closed, but it was possible for its worker goroutine to be scheduled so that the worker attempted to remove a reader from the map before the reader had been added to the map. This patch adds the item to the map before starting the goroutine which is expected to eventually remove it. Signed-off-by: Nalin Dahyabhai (github: nalind) --- daemon/logger/journald/read.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/daemon/logger/journald/read.go b/daemon/logger/journald/read.go index 8d94c302fb..fb95ea479e 100644 --- a/daemon/logger/journald/read.go +++ b/daemon/logger/journald/read.go @@ -173,6 +173,9 @@ drain: } func (s *journald) followJournal(logWatcher *logger.LogWatcher, config logger.ReadConfig, j *C.sd_journal, pfd [2]C.int, cursor string) { + s.readers.mu.Lock() + s.readers.readers[logWatcher] = logWatcher + s.readers.mu.Unlock() go func() { // Keep copying journal data out until we're notified to stop. for C.wait_for_data_or_close(j, pfd[0]) == 1 { @@ -184,9 +187,6 @@ func (s *journald) followJournal(logWatcher *logger.LogWatcher, config logger.Re delete(s.readers.readers, logWatcher) s.readers.mu.Unlock() }() - s.readers.mu.Lock() - s.readers.readers[logWatcher] = logWatcher - s.readers.mu.Unlock() // Wait until we're told to stop. select { case <-logWatcher.WatchClose(): From 52c0f36f7b7aa794932fa41dfe50dc85f78e6146 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Thu, 17 Mar 2016 16:19:54 -0400 Subject: [PATCH 2/2] Fix a race in cleaning up after journald followers When following a journal-based log, it was possible for the worker goroutine, which reads the journal using the journal context and sends entry data down the message channel, to be scheduled after the function which started it had returned. This could create problems, since the invoking function was closing the journal context object and message channel before it returned, which could trigger use-after-free segfaults and write-to-closed-channel panics in the worker goroutine. Make the cleanup in the invoking function conditional so that it's only done when we're not following the logs, and if we are, that it's left to the worker goroutine to close them. Signed-off-by: Nalin Dahyabhai (github: nalind) --- daemon/logger/journald/read.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/daemon/logger/journald/read.go b/daemon/logger/journald/read.go index fb95ea479e..3364160b9e 100644 --- a/daemon/logger/journald/read.go +++ b/daemon/logger/journald/read.go @@ -186,6 +186,8 @@ func (s *journald) followJournal(logWatcher *logger.LogWatcher, config logger.Re s.readers.mu.Lock() delete(s.readers.readers, logWatcher) s.readers.mu.Unlock() + C.sd_journal_close(j) + close(logWatcher.Msg) }() // Wait until we're told to stop. select { @@ -203,14 +205,24 @@ func (s *journald) readLogs(logWatcher *logger.LogWatcher, config logger.ReadCon var pipes [2]C.int cursor := "" - defer close(logWatcher.Msg) // Get a handle to the journal. rc := C.sd_journal_open(&j, C.int(0)) if rc != 0 { logWatcher.Err <- fmt.Errorf("error opening journal") + close(logWatcher.Msg) return } - defer C.sd_journal_close(j) + // If we end up following the log, we can set the journal context + // pointer and the channel pointer to nil so that we won't close them + // here, potentially while the goroutine that uses them is still + // running. Otherwise, close them when we return from this function. + following := false + defer func(pfollowing *bool) { + if !*pfollowing { + C.sd_journal_close(j) + close(logWatcher.Msg) + } + }(&following) // Remove limits on the size of data items that we'll retrieve. rc = C.sd_journal_set_data_threshold(j, C.size_t(0)) if rc != 0 { @@ -286,6 +298,9 @@ func (s *journald) readLogs(logWatcher *logger.LogWatcher, config logger.ReadCon logWatcher.Err <- fmt.Errorf("error opening journald close notification pipe") } else { s.followJournal(logWatcher, config, j, pipes, cursor) + // Let followJournal handle freeing the journal context + // object and closing the channel. + following = true } } return