From 6e6672c921d72cf6208572d3a648a67ecd324782 Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Mon, 8 Aug 2022 23:16:34 +0200 Subject: [PATCH 1/4] tail: fix race condition (fix #3765) There exists a race condition (RC) that can occur if changes to a path happen after the initial print loop in `uu_tail()`, but before the path is added to the notify-Watcher thread in `follow()`. To minimize the window where the RC can occur, this moves starting the Watcher thread and adding paths to it from `follow()` to the initial print loop in `uu_tail()`. Additionally, to make sure the RC cannot happen in "gnu/tests/tail-2/F-headers.sh", the error message that is used as a trigger in this test, is delayed until the path is added to the Watcher thread. --- src/uu/tail/src/tail.rs | 240 +++++++++++++++++++++++----------------- 1 file changed, 137 insertions(+), 103 deletions(-) diff --git a/src/uu/tail/src/tail.rs b/src/uu/tail/src/tail.rs index 7ac33f311e8..dde028b3938 100644 --- a/src/uu/tail/src/tail.rs +++ b/src/uu/tail/src/tail.rs @@ -36,7 +36,7 @@ use std::fmt; use std::fs::{File, Metadata}; use std::io::{stdin, stdout, BufRead, BufReader, Read, Seek, SeekFrom, Write}; use std::path::{Path, PathBuf}; -use std::sync::mpsc::{self, channel}; +use std::sync::mpsc::{self, channel, Receiver}; use std::time::Duration; use uucore::display::Quotable; use uucore::error::{ @@ -325,9 +325,17 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { let mut first_header = true; let mut files = FileHandling::with_capacity(settings.paths.len()); + let mut watcher_rx = if settings.follow.is_some() { + let (watcher, rx) = start_watcher_thread(&mut settings)?; + Some((watcher, rx)) + } else { + None + }; // Do an initial tail print of each path's content. // Add `path` to `files` map if `--follow` is selected. + // Add existing regular files to `Watcher` (InotifyWatcher). + // If `path` is not an existing file, add its parent to `Watcher`. for path in &settings.paths { let mut path = path.to_owned(); let mut display_name = path.to_owned(); @@ -355,11 +363,13 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { if !path.exists() && !settings.stdin_is_pipe_or_fifo { set_exit_code(1); - show_error!( - "cannot open {} for reading: {}", - display_name.quote(), - text::NO_SUCH_FILE - ); + if watcher_rx.is_none() { + show_error!( + "cannot open {} for reading: {}", + display_name.quote(), + text::NO_SUCH_FILE + ); + } } else if path.is_dir() || display_name.is_stdin() && !stdin_read_possible { if settings.verbose { files.print_header(&display_name, !first_header); @@ -458,6 +468,9 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { unbounded_tail(&mut reader, &settings)?; } if settings.follow.is_some() { + if let Some((ref mut watcher, _)) = watcher_rx { + watcher.watch_with_parent(&path)?; + } // Insert existing/file `path` into `files.map` files.insert( &path, @@ -481,20 +494,40 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { })); } } - } else if settings.retry && settings.follow.is_some() { - if path.is_relative() { - path = std::env::current_dir()?.join(&path); + } else { + if settings.retry && settings.follow.is_some() { + if path.is_relative() { + path = std::env::current_dir()?.join(&path); + } + if !path.is_orphan() { + if let Some((ref mut watcher, _)) = watcher_rx { + watcher.watch(path.parent().unwrap(), RecursiveMode::NonRecursive).unwrap(); + } + } + // Insert non-is_tailable() paths into `files.map` + files.insert( + &path, + PathData { + reader: None, + metadata, + display_name: display_name.to_owned(), + }, + false, + ); + } + if !path.exists() && !settings.stdin_is_pipe_or_fifo { + if watcher_rx.is_some() { + // NOTE: This error message is used as a trigger in + // "gnu/tests/tail-2/F-headers.sh". To prevent a possible + // race condition, we delay showing it until this path + // is added to the watcher thread. + show_error!( + "cannot open {} for reading: {}", + display_name.quote(), + text::NO_SUCH_FILE + ); + } } - // Insert non-is_tailable() paths into `files.map` - files.insert( - &path, - PathData { - reader: None, - metadata, - display_name, - }, - false, - ); } } @@ -513,7 +546,7 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { show_error!("{}", text::NO_FILES_REMAINING); } } else if !(settings.stdin_is_pipe_or_fifo && settings.paths.len() == 1) { - follow(&mut files, &mut settings)?; + follow(&mut files, &mut settings, watcher_rx)?; } } @@ -526,7 +559,7 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { fn arg_iterate<'a>( mut args: impl uucore::Args + 'a, -) -> Result + 'a>, Box<(dyn UError + 'static)>> { +) -> Result + 'a>, Box<(dyn UError)>> { // argv[0] is always present let first = args.next().unwrap(); if let Some(second) = args.next() { @@ -675,84 +708,24 @@ pub fn uu_app<'a>() -> Command<'a> { ) } -fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { +fn follow( + files: &mut FileHandling, + settings: &mut Settings, + watcher_rx: Option<( + Box, + Receiver>, + )>, +) -> UResult<()> { + let (mut watcher, rx) = watcher_rx.unwrap(); let mut process = platform::ProcessChecker::new(settings.pid); - let (tx, rx) = channel(); - - /* - Watcher is implemented per platform using the best implementation available on that - platform. In addition to such event driven implementations, a polling implementation - is also provided that should work on any platform. - Linux / Android: inotify - macOS: FSEvents / kqueue - Windows: ReadDirectoryChangesWatcher - FreeBSD / NetBSD / OpenBSD / DragonflyBSD: kqueue - Fallback: polling every n seconds - - NOTE: - We force the use of kqueue with: features=["macos_kqueue"]. - On macOS only `kqueue` is suitable for our use case because `FSEvents` - waits for file close util it delivers a modify event. See: - https://github.com/notify-rs/notify/issues/240 - */ - - let mut watcher: Box; - let watcher_config = notify::poll::PollWatcherConfig { - poll_interval: settings.sleep_sec, - /* - NOTE: By enabling compare_contents, performance will be significantly impacted - as all files will need to be read and hashed at each `poll_interval`. - However, this is necessary to pass: "gnu/tests/tail-2/F-vs-rename.sh" - */ - compare_contents: true, - }; - if settings.use_polling || RecommendedWatcher::kind() == WatcherKind::PollWatcher { - settings.use_polling = true; // We have to use polling because there's no supported backend - watcher = Box::new(notify::PollWatcher::with_config(tx, watcher_config).unwrap()); - } else { - let tx_clone = tx.clone(); - match notify::RecommendedWatcher::new(tx) { - Ok(w) => watcher = Box::new(w), - Err(e) if e.to_string().starts_with("Too many open files") => { - /* - NOTE: This ErrorKind is `Uncategorized`, but it is not recommended - to match an error against `Uncategorized` - NOTE: Could be tested with decreasing `max_user_instances`, e.g.: - `sudo sysctl fs.inotify.max_user_instances=64` - */ - show_error!( - "{} cannot be used, reverting to polling: Too many open files", - text::BACKEND - ); - set_exit_code(1); - settings.use_polling = true; - watcher = - Box::new(notify::PollWatcher::with_config(tx_clone, watcher_config).unwrap()); - } - Err(e) => return Err(USimpleError::new(1, e.to_string())), - }; - } - // Iterate user provided `paths`. - // Add existing regular files to `Watcher` (InotifyWatcher). - // If `path` is not an existing file, add its parent to `Watcher`. // If there is no parent, add `path` to `orphans`. - let mut orphans = Vec::new(); - for path in files.keys() { - if path.is_tailable() { - watcher.watch_with_parent(path)?; - } else if settings.follow.is_some() && settings.retry { - if path.is_orphan() { - orphans.push(path.to_owned()); - } else { - watcher.watch_with_parent(path.parent().unwrap())?; - } - } else { - // TODO: [2022-05; jhscheer] do we need to handle this case? - unimplemented!(); - } - } + let mut orphans: Vec = files + .keys() + .filter(|p| p.is_orphan()) + .map(|p| p.to_owned()) + .collect(); // TODO: [2021-10; jhscheer] let mut _event_counter = 0; @@ -865,6 +838,73 @@ fn follow(files: &mut FileHandling, settings: &mut Settings) -> UResult<()> { Ok(()) } +fn start_watcher_thread( + settings: &mut Settings, +) -> Result< + ( + Box<(dyn Watcher)>, + Receiver>, + ), + Box<(dyn UError)>, +> { + let (tx, rx) = channel(); + + /* + Watcher is implemented per platform using the best implementation available on that + platform. In addition to such event driven implementations, a polling implementation + is also provided that should work on any platform. + Linux / Android: inotify + macOS: FSEvents / kqueue + Windows: ReadDirectoryChangesWatcher + FreeBSD / NetBSD / OpenBSD / DragonflyBSD: kqueue + Fallback: polling every n seconds + + NOTE: + We force the use of kqueue with: features=["macos_kqueue"]. + On macOS only `kqueue` is suitable for our use case because `FSEvents` + waits for file close util it delivers a modify event. See: + https://github.com/notify-rs/notify/issues/240 + */ + + let watcher: Box; + let watcher_config = notify::poll::PollWatcherConfig { + poll_interval: settings.sleep_sec, + /* + NOTE: By enabling compare_contents, performance will be significantly impacted + as all files will need to be read and hashed at each `poll_interval`. + However, this is necessary to pass: "gnu/tests/tail-2/F-vs-rename.sh" + */ + compare_contents: true, + }; + if settings.use_polling || RecommendedWatcher::kind() == WatcherKind::PollWatcher { + settings.use_polling = true; // We have to use polling because there's no supported backend + watcher = Box::new(notify::PollWatcher::with_config(tx, watcher_config).unwrap()); + } else { + let tx_clone = tx.clone(); + match notify::RecommendedWatcher::new(tx) { + Ok(w) => watcher = Box::new(w), + Err(e) if e.to_string().starts_with("Too many open files") => { + /* + NOTE: This ErrorKind is `Uncategorized`, but it is not recommended + to match an error against `Uncategorized` + NOTE: Could be tested with decreasing `max_user_instances`, e.g.: + `sudo sysctl fs.inotify.max_user_instances=64` + */ + show_error!( + "{} cannot be used, reverting to polling: Too many open files", + text::BACKEND + ); + set_exit_code(1); + settings.use_polling = true; + watcher = + Box::new(notify::PollWatcher::with_config(tx_clone, watcher_config).unwrap()); + } + Err(e) => return Err(USimpleError::new(1, e.to_string())), + }; + } + Ok((watcher, rx)) +} + fn handle_event( event: ¬ify::Event, files: &mut FileHandling, @@ -1521,10 +1561,7 @@ impl FileExtTail for File { trait MetadataExtTail { fn is_tailable(&self) -> bool; - fn got_truncated( - &self, - other: &Metadata, - ) -> Result>; + fn got_truncated(&self, other: &Metadata) -> Result>; fn get_block_size(&self) -> u64; fn file_id_eq(&self, other: &Metadata) -> bool; } @@ -1543,10 +1580,7 @@ impl MetadataExtTail for Metadata { } /// Return true if the file was modified and is now shorter - fn got_truncated( - &self, - other: &Metadata, - ) -> Result> { + fn got_truncated(&self, other: &Metadata) -> Result> { Ok(other.len() < self.len() && other.modified()? != self.modified()?) } From 24b19c638e67fa31be38649bec55a4681a7f405f Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Tue, 9 Aug 2022 00:01:18 +0200 Subject: [PATCH 2/4] build-gnu: remove workarounds for tail Remove workarounds for "tests/tail-2/F-headers.sh" which are (presumably) no longer needed because of the race condition fix. --- util/build-gnu.sh | 4 ---- 1 file changed, 4 deletions(-) diff --git a/util/build-gnu.sh b/util/build-gnu.sh index c875d4b3e70..b3b27627c3c 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -168,10 +168,6 @@ sed -i -e "s|removed directory 'a/'|removed directory 'a'|g" tests/rm/v-slash.sh # however there's a bug because `---dis` is an alias for: `---disable-inotify` sed -i -e "s|---dis ||g" tests/tail-2/overlay-headers.sh -# F-headers.sh test sometime fails (but only in CI), -# just testing inotify should make it more stable -sed -i -e "s|echo x > a|sleep .1; echo x > a; sleep .1|g" -e "s|echo y > b|sleep .1; echo y > b; sleep .1|g" -e "s| '---disable-inotify'||g" tests/tail-2/F-headers.sh - test -f "${UU_BUILD_DIR}/getlimits" || cp src/getlimits "${UU_BUILD_DIR}" # When decoding an invalid base32/64 string, gnu writes everything it was able to decode until From 3a25def6dcd427f07ebacf095ea4b99faf8d4bfd Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Tue, 9 Aug 2022 00:01:18 +0200 Subject: [PATCH 3/4] build-gnu: remove workarounds for tail Remove workarounds for "tests/tail-2/F-headers.sh" which are (presumably) no longer needed because of the race condition fix. --- src/uu/tail/src/tail.rs | 4 +++- util/build-gnu.sh | 4 ---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/uu/tail/src/tail.rs b/src/uu/tail/src/tail.rs index dde028b3938..c7e3c833974 100644 --- a/src/uu/tail/src/tail.rs +++ b/src/uu/tail/src/tail.rs @@ -501,7 +501,9 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { } if !path.is_orphan() { if let Some((ref mut watcher, _)) = watcher_rx { - watcher.watch(path.parent().unwrap(), RecursiveMode::NonRecursive).unwrap(); + watcher + .watch(path.parent().unwrap(), RecursiveMode::NonRecursive) + .unwrap(); } } // Insert non-is_tailable() paths into `files.map` diff --git a/util/build-gnu.sh b/util/build-gnu.sh index c875d4b3e70..b3b27627c3c 100755 --- a/util/build-gnu.sh +++ b/util/build-gnu.sh @@ -168,10 +168,6 @@ sed -i -e "s|removed directory 'a/'|removed directory 'a'|g" tests/rm/v-slash.sh # however there's a bug because `---dis` is an alias for: `---disable-inotify` sed -i -e "s|---dis ||g" tests/tail-2/overlay-headers.sh -# F-headers.sh test sometime fails (but only in CI), -# just testing inotify should make it more stable -sed -i -e "s|echo x > a|sleep .1; echo x > a; sleep .1|g" -e "s|echo y > b|sleep .1; echo y > b; sleep .1|g" -e "s| '---disable-inotify'||g" tests/tail-2/F-headers.sh - test -f "${UU_BUILD_DIR}/getlimits" || cp src/getlimits "${UU_BUILD_DIR}" # When decoding an invalid base32/64 string, gnu writes everything it was able to decode until From 85bfef86ca20d589e5b34d9808c9b351a89b97bb Mon Sep 17 00:00:00 2001 From: Jan Scheer Date: Thu, 11 Aug 2022 15:13:53 +0200 Subject: [PATCH 4/4] tail: refactor to minimize chances of RC Move "adding paths to Watcher thread" to its own loop and run this loop before the initial tail-print-loop in order to minimize the window for race conditions. --- src/uu/tail/src/tail.rs | 148 +++++++++++++++++++++------------------- 1 file changed, 76 insertions(+), 72 deletions(-) diff --git a/src/uu/tail/src/tail.rs b/src/uu/tail/src/tail.rs index b46920668c8..727137b9292 100644 --- a/src/uu/tail/src/tail.rs +++ b/src/uu/tail/src/tail.rs @@ -323,8 +323,14 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { settings.paths.push_front(dash); } + // TODO: is there a better way to check for a readable stdin? + let mut buf = [0; 0]; // empty buffer to check if stdin().read().is_err() + let stdin_read_possible = settings.stdin_is_pipe_or_fifo && stdin().read(&mut buf).is_ok(); + let mut first_header = true; let mut files = FileHandling::with_capacity(settings.paths.len()); + let mut orphans = Vec::new(); + let mut watcher_rx = if settings.follow.is_some() { let (watcher, rx) = start_watcher_thread(&mut settings)?; Some((watcher, rx)) @@ -332,28 +338,45 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { None }; - // Do an initial tail print of each path's content. - // Add `path` to `files` map if `--follow` is selected. - // Add existing regular files to `Watcher` (InotifyWatcher). - // If `path` is not an existing file, add its parent to `Watcher`. - for path in &settings.paths { - let mut path = path.to_owned(); - let mut display_name = path.to_owned(); + // Iterate user provided `paths` and add them to Watcher. + if let Some((ref mut watcher, _)) = watcher_rx { + for path in &settings.paths { + let mut path = path.handle_redirect(); + if path.is_stdin() { + continue; + } + #[cfg(all(unix, not(target_os = "linux")))] + if !path.is_file() { + continue; + } + if path.is_relative() { + path = std::env::current_dir()?.join(&path); + } - // Workaround to handle redirects, e.g. `touch f && tail -f - < f` - if cfg!(unix) && path.is_stdin() { - display_name = PathBuf::from(text::STDIN_HEADER); - if let Ok(p) = Path::new(text::DEV_STDIN).canonicalize() { - path = p; + if path.is_tailable() { + // Add existing regular files to `Watcher` (InotifyWatcher). + watcher.watch_with_parent(&path)?; + } else if !path.is_orphan() { + // If `path` is not a tailable file, add its parent to `Watcher`. + watcher + .watch(path.parent().unwrap(), RecursiveMode::NonRecursive) + .unwrap(); } else { - path = PathBuf::from(text::DEV_STDIN); + // If there is no parent, add `path` to `orphans`. + orphans.push(path.to_owned()); } } + } - // TODO: is there a better way to check for a readable stdin? - let mut buf = [0; 0]; // empty buffer to check if stdin().read().is_err() - let stdin_read_possible = settings.stdin_is_pipe_or_fifo && stdin().read(&mut buf).is_ok(); - + // Do an initial tail print of each path's content. + // Add `path` and `reader` to `files` map if `--follow` is selected. + for path in &settings.paths { + let display_name = if cfg!(unix) && path.is_stdin() { + PathBuf::from(text::STDIN_HEADER) + } else { + path.to_owned() + }; + let mut path = path.handle_redirect(); let path_is_tailable = path.is_tailable(); if !path.is_stdin() && !path_is_tailable { @@ -363,13 +386,11 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { if !path.exists() && !settings.stdin_is_pipe_or_fifo { set_exit_code(1); - if watcher_rx.is_none() { - show_error!( - "cannot open {} for reading: {}", - display_name.quote(), - text::NO_SUCH_FILE - ); - } + show_error!( + "cannot open {} for reading: {}", + display_name.quote(), + text::NO_SUCH_FILE + ); } else if path.is_dir() || display_name.is_stdin() && !stdin_read_possible { if settings.verbose { files.print_header(&display_name, !first_header); @@ -468,9 +489,6 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { unbounded_tail(&mut reader, &settings)?; } if settings.follow.is_some() { - if let Some((ref mut watcher, _)) = watcher_rx { - watcher.watch_with_parent(&path)?; - } // Insert existing/file `path` into `files.map` files.insert( &path, @@ -494,40 +512,20 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { })); } } - } else { - if settings.retry && settings.follow.is_some() { - if path.is_relative() { - path = std::env::current_dir()?.join(&path); - } - if !path.is_orphan() { - if let Some((ref mut watcher, _)) = watcher_rx { - watcher - .watch(path.parent().unwrap(), RecursiveMode::NonRecursive) - .unwrap(); - } - } - // Insert non-is_tailable() paths into `files.map` - files.insert( - &path, - PathData { - reader: None, - metadata, - display_name: display_name.to_owned(), - }, - false, - ); - } - if !path.exists() && !settings.stdin_is_pipe_or_fifo && watcher_rx.is_some() { - // NOTE: This error message is used as a trigger in - // "gnu/tests/tail-2/F-headers.sh". To prevent a possible - // race condition, we delay showing it until this path - // is added to the watcher thread. - show_error!( - "cannot open {} for reading: {}", - display_name.quote(), - text::NO_SUCH_FILE - ); + } else if settings.retry && settings.follow.is_some() { + if path.is_relative() { + path = std::env::current_dir()?.join(&path); } + // Insert non-is_tailable() paths into `files.map` + files.insert( + &path, + PathData { + reader: None, + metadata, + display_name, + }, + false, + ); } } @@ -546,7 +544,7 @@ fn uu_tail(mut settings: Settings) -> UResult<()> { show_error!("{}", text::NO_FILES_REMAINING); } } else if !(settings.stdin_is_pipe_or_fifo && settings.paths.len() == 1) { - follow(&mut files, &mut settings, watcher_rx)?; + follow(files, &settings, watcher_rx, orphans)?; } } @@ -714,21 +712,14 @@ type WatcherRx = ( ); fn follow( - files: &mut FileHandling, - settings: &mut Settings, + mut files: FileHandling, + settings: &Settings, watcher_rx: Option, + mut orphans: Vec, ) -> UResult<()> { let (mut watcher, rx) = watcher_rx.unwrap(); let mut process = platform::ProcessChecker::new(settings.pid); - // Iterate user provided `paths`. - // If there is no parent, add `path` to `orphans`. - let mut orphans: Vec = files - .keys() - .filter(|p| p.is_orphan()) - .map(|p| p.to_owned()) - .collect(); - // TODO: [2021-10; jhscheer] let mut _event_counter = 0; let mut _timeout_counter = 0; @@ -781,7 +772,8 @@ fn follow( if let Some(event_path) = event.paths.first() { if files.contains_key(event_path) { // Handle Event if it is about a path that we are monitoring - paths = handle_event(&event, files, settings, &mut watcher, &mut orphans)?; + paths = + handle_event(&event, &mut files, settings, &mut watcher, &mut orphans)?; } } } @@ -1613,6 +1605,7 @@ trait PathExtTail { fn is_stdin(&self) -> bool; fn is_orphan(&self) -> bool; fn is_tailable(&self) -> bool; + fn handle_redirect(&self) -> PathBuf; } impl PathExtTail for Path { @@ -1631,6 +1624,17 @@ impl PathExtTail for Path { fn is_tailable(&self) -> bool { self.is_file() || self.exists() && self.metadata().unwrap().is_tailable() } + /// Workaround to handle redirects, e.g. `touch f && tail -f - < f` + fn handle_redirect(&self) -> PathBuf { + if cfg!(unix) && self.is_stdin() { + if let Ok(p) = Self::new(text::DEV_STDIN).canonicalize() { + return p; + } else { + return PathBuf::from(text::DEV_STDIN); + } + } + self.into() + } } trait WatcherExtTail {