From e9d6808ca6bfeb6b8d1ca30113c186098e4d2d56 Mon Sep 17 00:00:00 2001 From: Andy Zhou Date: Fri, 13 Nov 2015 18:39:37 -0800 Subject: vlog: Fix a deadlock bug. Calling VLOG_FATAL() while holding the 'log_file_mutex" may lead to deadlock since VLOG_FATAL() implementation tries to acquire the same lock. Fix this by building the error message first, then call VLOG_FATAL() after the 'log_file_mutex' has been released. This bug is not likely show up in practice since chown() usually won't fail. It is still better to have a correct implementation. Reported-by: Daniele Di Proietto Signed-off-by: Andy Zhou Acked-by: Daniele Di Proietto --- lib/vlog.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'lib/vlog.c') diff --git a/lib/vlog.c b/lib/vlog.c index a4aa2a090..28cea5ddb 100644 --- a/lib/vlog.c +++ b/lib/vlog.c @@ -438,15 +438,24 @@ vlog_reopen_log_file(void) void vlog_change_owner_unix(uid_t user, gid_t group) { - ovs_mutex_lock(&log_file_mutex); - int error = log_file_name ? chown(log_file_name, user, group) : 0; + struct ds err = DS_EMPTY_INITIALIZER; + int error; + ovs_mutex_lock(&log_file_mutex); + error = log_file_name ? chown(log_file_name, user, group) : 0; if (error) { - VLOG_FATAL("Failed to change %s ownership: %s.", - log_file_name, ovs_strerror(errno)); + /* Build the error message. We can not call VLOG_FATAL directly + * here because VLOG_FATAL() will try again to to acquire + * 'log_file_mutex' lock, causing deadlock. + */ + ds_put_format(&err, "Failed to change %s ownership: %s.", + log_file_name, ovs_strerror(errno)); } - ovs_mutex_unlock(&log_file_mutex); + + if (error) { + VLOG_FATAL("%s", ds_steal_cstr(&err)); + } } #endif -- cgit v1.2.1