Add timeouts for commit hooks

CHANGELOG.md

@@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * new command-line option to override the default log file path (`--logfile`) [[@acuteenvy](https://github.com/acuteenvy)] ([#2539](https://github.com/gitui-org/gitui/pull/2539))
 * dx: `make check` checks Cargo.toml dependency ordering using `cargo sort` [[@naseschwarz](https://github.com/naseschwarz)]
 * add `use_selection_fg` to theme file to allow customizing selection foreground color [[@Upsylonbare](https://github.com/Upsylonbare)] ([#2515](https://github.com/gitui-org/gitui/pull/2515))
+* add timeouts for hooks [[@DaRacci](https://github.com/DaRacci)] ([#2547](https://github.com/gitui-org/gitui/pull/2547))
 
 ### Changed
 * improve error messages [[@acuteenvy](https://github.com/acuteenvy)] ([#2617](https://github.com/gitui-org/gitui/pull/2617))

asyncgit/src/sync/hooks.rs

@@ -2,6 +2,7 @@ use super::{repository::repo, RepoPath};
 use crate::error::Result;
 pub use git2_hooks::PrepareCommitMsgSource;
 use scopetime::scope_time;
+use std::time::Duration;
 
 ///
 #[derive(Debug, PartialEq, Eq)]
@@ -10,6 +11,13 @@ pub enum HookResult {
 	Ok,
 	/// Hook returned error
 	NotOk(String),
+	/// Hook timed out
+	TimedOut {
+		/// Stdout
+		stdout: String,
+		/// Stderr
+		stderr: String,
+	},
 }
 
 impl From<git2_hooks::HookResult> for HookResult {
@@ -22,6 +30,11 @@ impl From<git2_hooks::HookResult> for HookResult {
 				stderr,
 				..
 			} => Self::NotOk(format!("{stdout}{stderr}")),
+			git2_hooks::HookResult::TimedOut {
+				stdout,
+				stderr,
+				..
+			} => Self::TimedOut { stdout, stderr },
 		}
 	}
 }
@@ -30,30 +43,66 @@ impl From<git2_hooks::HookResult> for HookResult {
 pub fn hooks_commit_msg(
 	repo_path: &RepoPath,
 	msg: &mut String,
+) -> Result<HookResult> {
+	hooks_commit_msg_with_timeout(repo_path, msg, None)
+}
+
+/// see `git2_hooks::hooks_commit_msg`
+#[allow(unused)]
+pub fn hooks_commit_msg_with_timeout(
+	repo_path: &RepoPath,
+	msg: &mut String,
+	timeout: Option<Duration>,
 ) -> Result<HookResult> {
 	scope_time!("hooks_commit_msg");
 
 	let repo = repo(repo_path)?;
-
-	Ok(git2_hooks::hooks_commit_msg(&repo, None, msg)?.into())
+	Ok(git2_hooks::hooks_commit_msg_with_timeout(
+		&repo, None, msg, timeout,
+	)?
+	.into())
 }
 
 /// see `git2_hooks::hooks_pre_commit`
 pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result<HookResult> {
+	hooks_pre_commit_with_timeout(repo_path, None)
+}
+
+/// see `git2_hooks::hooks_pre_commit`
+#[allow(unused)]
+pub fn hooks_pre_commit_with_timeout(
+	repo_path: &RepoPath,
+	timeout: Option<Duration>,
+) -> Result<HookResult> {
 	scope_time!("hooks_pre_commit");
 
 	let repo = repo(repo_path)?;
 
-	Ok(git2_hooks::hooks_pre_commit(&repo, None)?.into())
+	Ok(git2_hooks::hooks_pre_commit_with_timeout(
+		&repo, None, timeout,
+	)?
+	.into())
 }
 
 /// see `git2_hooks::hooks_post_commit`
 pub fn hooks_post_commit(repo_path: &RepoPath) -> Result<HookResult> {
+	hooks_post_commit_with_timeout(repo_path, None)
+}
+
+/// see `git2_hooks::hooks_post_commit`
+#[allow(unused)]
+pub fn hooks_post_commit_with_timeout(
+	repo_path: &RepoPath,
+	timeout: Option<Duration>,
+) -> Result<HookResult> {
 	scope_time!("hooks_post_commit");
 
 	let repo = repo(repo_path)?;
 
-	Ok(git2_hooks::hooks_post_commit(&repo, None)?.into())
+	Ok(git2_hooks::hooks_post_commit_with_timeout(
+		&repo, None, timeout,
+	)?
+	.into())
 }
 
 /// see `git2_hooks::hooks_prepare_commit_msg`
@@ -66,8 +115,26 @@ pub fn hooks_prepare_commit_msg(
 
 	let repo = repo(repo_path)?;
 
-	Ok(git2_hooks::hooks_prepare_commit_msg(
-		&repo, None, source, msg,
+	Ok(git2_hooks::hooks_prepare_commit_msg_with_timeout(
+		&repo, None, source, msg, None,
+	)?
+	.into())
+}
+
+/// see `git2_hooks::hooks_prepare_commit_msg`
+#[allow(unused)]
+pub fn hooks_prepare_commit_msg_with_timeout(
+	repo_path: &RepoPath,
+	source: PrepareCommitMsgSource,
+	msg: &mut String,
+	timeout: Option<Duration>,
+) -> Result<HookResult> {
+	scope_time!("hooks_prepare_commit_msg");
+
+	let repo = repo(repo_path)?;
+
+	Ok(git2_hooks::hooks_prepare_commit_msg_with_timeout(
+		&repo, None, source, msg, timeout,
 	)?
 	.into())
 }
@@ -77,7 +144,7 @@ mod tests {
 	use std::{ffi::OsString, io::Write as _, path::Path};
 
 	use git2::Repository;
-	use tempfile::TempDir;
+	use tempfile::{tempdir, TempDir};
 
 	use super::*;
 	use crate::sync::tests::repo_init_with_prefix;
@@ -125,7 +192,7 @@ mod tests {
 		let (_td, repo) = repo_init().unwrap();
 		let root = repo.workdir().unwrap();
 
-		let hook = b"#!/bin/sh
+		let hook = b"#!/usr/bin/env sh
 	echo 'rejected'
 	exit 1
 			";
@@ -239,4 +306,144 @@ mod tests {
 
 		assert_eq!(msg, String::from("msg\n"));
 	}
+
+	#[test]
+	fn test_hooks_respect_timeout() {
+		let (_td, repo) = repo_init().unwrap();
+		let root = repo.path().parent().unwrap();
+
+		let hook = b"#!/usr/bin/env sh
+    sleep 0.250
+        ";
+
+		git2_hooks::create_hook(
+			&repo,
+			git2_hooks::HOOK_PRE_COMMIT,
+			hook,
+		);
+
+		let res = hooks_pre_commit_with_timeout(
+			&root.to_str().unwrap().into(),
+			Some(Duration::from_millis(200)),
+		)
+		.unwrap();
+
+		assert_eq!(
+			res,
+			HookResult::TimedOut {
+				stdout: String::new(),
+				stderr: String::new()
+			}
+		);
+	}
+
+	#[test]
+	fn test_hooks_faster_than_timeout() {
+		let (_td, repo) = repo_init().unwrap();
+		let root = repo.path().parent().unwrap();
+
+		let hook = b"#!/usr/bin/env sh
+    sleep 0.1
+        ";
+
+		git2_hooks::create_hook(
+			&repo,
+			git2_hooks::HOOK_PRE_COMMIT,
+			hook,
+		);
+
+		let res = hooks_pre_commit_with_timeout(
+			&root.to_str().unwrap().into(),
+			Some(Duration::from_millis(150)),
+		)
+		.unwrap();
+
+		assert_eq!(res, HookResult::Ok);
+	}
+
+	#[test]
+	fn test_hooks_timeout_zero() {
+		let (_td, repo) = repo_init().unwrap();
+		let root = repo.path().parent().unwrap();
+
+		let hook = b"#!/usr/bin/env sh
+    sleep 1
+        ";
+
+		git2_hooks::create_hook(
+			&repo,
+			git2_hooks::HOOK_POST_COMMIT,
+			hook,
+		);
+
+		let res = hooks_post_commit_with_timeout(
+			&root.to_str().unwrap().into(),
+			Some(Duration::ZERO),
+		)
+		.unwrap();
+
+		assert_eq!(res, HookResult::Ok);
+	}
+
+	#[test]
+	fn test_run_with_timeout_kills() {
+		let (_td, repo) = repo_init().unwrap();
+		let root = repo.path().parent().unwrap();
+
+		let temp_dir = tempdir().expect("temp dir");
+		let file = temp_dir.path().join("test");
+		let hook = format!(
+			"#!/usr/bin/env sh
+sleep 5
+echo 'after sleep' > {}
+        ",
+			file.as_path().to_str().unwrap()
+		);
+
+		git2_hooks::create_hook(
+			&repo,
+			git2_hooks::HOOK_PRE_COMMIT,
+			hook.as_bytes(),
+		);
+
+		let res = hooks_pre_commit_with_timeout(
+			&root.to_str().unwrap().into(),
+			Some(Duration::from_millis(100)),
+		);
+
+		assert!(res.is_ok());
+		assert!(!file.exists());
+	}
+
+	#[test]
+	#[cfg(unix)]
+	fn test_ensure_group_kill_works() {
+		let (_td, repo) = repo_init().unwrap();
+		let root = repo.path().parent().unwrap();
+
+		let hook = b"#!/usr/bin/env sh
+sleep 30
+		";
+
+		git2_hooks::create_hook(
+			&repo,
+			git2_hooks::HOOK_PRE_COMMIT,
+			hook,
+		);
+
+		let time_start = std::time::Instant::now();
+		let res = hooks_pre_commit_with_timeout(
+			&root.to_str().unwrap().into(),
+			Some(Duration::from_millis(150)),
+		)
+		.unwrap();
+		let time_end = std::time::Instant::now();
+		let elapsed = time_end.duration_since(time_start);
+
+		log::info!("elapsed: {:?}", elapsed);
+		// If the children didn't get killed this would
+		// have taken the full 30 seconds.
+		assert!(elapsed.as_secs() < 15);
+		assert!(matches!(res, HookResult::TimedOut { .. }))
+	}
 }

asyncgit/src/sync/mod.rs

@@ -66,8 +66,11 @@ pub use config::{
 pub use diff::get_diff_commit;
 pub use git2::BranchType;
 pub use hooks::{
-	hooks_commit_msg, hooks_post_commit, hooks_pre_commit,
-	hooks_prepare_commit_msg, HookResult, PrepareCommitMsgSource,
+	hooks_commit_msg, hooks_commit_msg_with_timeout,
+	hooks_post_commit, hooks_post_commit_with_timeout,
+	hooks_pre_commit, hooks_pre_commit_with_timeout,
+	hooks_prepare_commit_msg, hooks_prepare_commit_msg_with_timeout,
+	HookResult, PrepareCommitMsgSource,
 };
 pub use hunks::{reset_hunk, stage_hunk, unstage_hunk};
 pub use ignore::add_to_ignore;

git2-hooks/Cargo.toml

@@ -18,6 +18,7 @@ gix-path = "0.10"
 log = "0.4"
 shellexpand = "3.1"
 thiserror = "2.0"
+nix = { version = "0.30.1", features = ["process", "signal"], default-features = false }
 
 [dev-dependencies]
 git2-testing = { path = "../git2-testing" }