Browse code

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 <nalin@redhat.com> (github: nalind)

Nalin Dahyabhai authored on 2016/03/18 05:19:54
Showing 1 changed files
... ...
@@ -186,6 +186,8 @@ func (s *journald) followJournal(logWatcher *logger.LogWatcher, config logger.Re
186 186
 		s.readers.mu.Lock()
187 187
 		delete(s.readers.readers, logWatcher)
188 188
 		s.readers.mu.Unlock()
189
+		C.sd_journal_close(j)
190
+		close(logWatcher.Msg)
189 191
 	}()
190 192
 	// Wait until we're told to stop.
191 193
 	select {
... ...
@@ -203,14 +205,24 @@ func (s *journald) readLogs(logWatcher *logger.LogWatcher, config logger.ReadCon
203 203
 	var pipes [2]C.int
204 204
 	cursor := ""
205 205
 
206
-	defer close(logWatcher.Msg)
207 206
 	// Get a handle to the journal.
208 207
 	rc := C.sd_journal_open(&j, C.int(0))
209 208
 	if rc != 0 {
210 209
 		logWatcher.Err <- fmt.Errorf("error opening journal")
210
+		close(logWatcher.Msg)
211 211
 		return
212 212
 	}
213
-	defer C.sd_journal_close(j)
213
+	// If we end up following the log, we can set the journal context
214
+	// pointer and the channel pointer to nil so that we won't close them
215
+	// here, potentially while the goroutine that uses them is still
216
+	// running.  Otherwise, close them when we return from this function.
217
+	following := false
218
+	defer func(pfollowing *bool) {
219
+		if !*pfollowing {
220
+			C.sd_journal_close(j)
221
+			close(logWatcher.Msg)
222
+		}
223
+	}(&following)
214 224
 	// Remove limits on the size of data items that we'll retrieve.
215 225
 	rc = C.sd_journal_set_data_threshold(j, C.size_t(0))
216 226
 	if rc != 0 {
... ...
@@ -286,6 +298,9 @@ func (s *journald) readLogs(logWatcher *logger.LogWatcher, config logger.ReadCon
286 286
 			logWatcher.Err <- fmt.Errorf("error opening journald close notification pipe")
287 287
 		} else {
288 288
 			s.followJournal(logWatcher, config, j, pipes, cursor)
289
+			// Let followJournal handle freeing the journal context
290
+			// object and closing the channel.
291
+			following = true
289 292
 		}
290 293
 	}
291 294
 	return