From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tyler Hicks Subject: [PATCH 2/3] Fix Wunused-return warnings Date: Fri, 8 Feb 2013 19:12:34 -0800 Message-ID: <1360379555-1910-3-git-send-email-tyhicks@canonical.com> References: <1360379555-1910-1-git-send-email-tyhicks@canonical.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1360379555-1910-1-git-send-email-tyhicks@canonical.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Steve Grubb Cc: linux-audit@redhat.com List-Id: linux-audit@redhat.com When building with -D_FORTIFY_SOURCE=2 and -W-unused-return, there are a number of warnings caused by return values of functions marked with the warn_unused_result attribute being ignored. The audit codebase makes an attempt to suppress these warnings by casting the return value to void, but that does not work when D_FORITY_SOURCE is in use. Here's an explanation of how this patch fixes the warnings and how the potential error conditions are handled: Errors writing to the auditd pid file should be logged since errors opening the pid file are logged. These write() errors aren't treated as fatal. Problems adjusting auditd's out of memory score should be logged, if simply to catch a change to the kernel interface. These errors aren't treated as fatal. Auditd refuses to start when nice() fails during initialization, so it should take disk_error_action whenever nice() fails during a reconfigure. Failure to chdir("/") while daemonizing should be logged and treated as fatal since errors while redirecting stdin, stdout, and stderr are logged and considered fatal. All nice() return values are handled sufficiently by relying on errno. However, they still throw warnings when D_FORTIFY_SOURCE is used. This patch quiets those warnings by capturing the return value and using it and errno to determine if nice() failed. Failure to adjust audit log file owner (fchown) and permissions (fchmod) are logged and considered fatal when opening the log file for the first time. They are not treated as fatal when the operations fail on during log rotation since we made sure that they file owner and permissions were correct when originally opening the log file. Signed-off-by: Tyler Hicks --- audisp/audispd.c | 6 ++++-- src/auditd-event.c | 45 +++++++++++++++++++++++++++++++++++++-------- src/auditd.c | 52 +++++++++++++++++++++++++++++++++------------------- src/autrace.c | 6 +++++- 4 files changed, 79 insertions(+), 30 deletions(-) diff --git a/audisp/audispd.c b/audisp/audispd.c index 749128f..0ef4b8e 100644 --- a/audisp/audispd.c +++ b/audisp/audispd.c @@ -369,9 +369,11 @@ int main(int argc, char *argv[]) /* Now boost priority to make sure we are getting time slices */ if (daemon_config.priority_boost != 0) { + int rc; + errno = 0; - (void) nice((int)-daemon_config.priority_boost); - if (errno) { + rc = nice((int)-daemon_config.priority_boost); + if (rc == -1 && errno) { syslog(LOG_ERR, "Cannot change priority (%s)", strerror(errno)); /* Stay alive as this is better than stopping */ diff --git a/src/auditd-event.c b/src/auditd-event.c index acf5aa1..2965be3 100644 --- a/src/auditd-event.c +++ b/src/auditd-event.c @@ -708,10 +708,18 @@ static void rotate_logs(struct auditd_consumer_data *data, if (data->config->num_logs < 2) return; - /* Close audit file */ - fchmod(data->log_fd, - data->config->log_group ? S_IRUSR|S_IRGRP : S_IRUSR); - fchown(data->log_fd, 0, data->config->log_group); + /* Close audit file. fchmod and fchown errors are not fatal because we + * already adjusted log file permissions and ownership when opening the + * log file. */ + if (fchmod(data->log_fd, data->config->log_group ? S_IRUSR|S_IRGRP : + S_IRUSR) < 0) { + audit_msg(LOG_NOTICE, "Couldn't change permissions while " + "rotating log file (%s)", strerror(errno)); + } + if (fchown(data->log_fd, 0, data->config->log_group) < 0) { + audit_msg(LOG_NOTICE, "Couldn't change ownership while " + "rotating log file (%s)", strerror(errno)); + } fclose(data->log_file); /* Rotate */ @@ -924,9 +932,20 @@ retry: return 1; } } - fchmod(lfd, data->config->log_group ? S_IRUSR|S_IWUSR|S_IRGRP : - S_IRUSR|S_IWUSR); - fchown(lfd, 0, data->config->log_group); + if (fchmod(lfd, data->config->log_group ? S_IRUSR|S_IWUSR|S_IRGRP : + S_IRUSR|S_IWUSR) < 0) { + audit_msg(LOG_ERR, + "Couldn't change permissions of log file (%s)", + strerror(errno)); + close(lfd); + return 1; + } + if (fchown(lfd, 0, data->config->log_group) < 0) { + audit_msg(LOG_ERR, "Couldn't change ownership of log file (%s)", + strerror(errno)); + close(lfd); + return 1; + } data->log_fd = lfd; data->log_file = fdopen(lfd, "a"); @@ -1089,8 +1108,18 @@ static void reconfigure(struct auditd_consumer_data *data) // priority boost if (oconf->priority_boost != nconf->priority_boost) { + int rc; + oconf->priority_boost = nconf->priority_boost; - nice(-oconf->priority_boost); + errno = 0; + rc = nice(-oconf->priority_boost); + if (rc == -1 && errno) { + int saved_errno = errno; + audit_msg(LOG_NOTICE, "Cannot change priority in " + "reconfigure (%s)", strerror(errno)); + do_disk_error_action("reconfig", data->config, + saved_errno); + } } // log format diff --git a/src/auditd.c b/src/auditd.c index 84fdea2..52ec5df 100644 --- a/src/auditd.c +++ b/src/auditd.c @@ -240,7 +240,7 @@ int send_audit_event(int type, const char *str) static int write_pid_file(void) { - int pidfd, len; + int pidfd, len, rc; char val[16]; len = snprintf(val, sizeof(val), "%u\n", getpid()); @@ -256,29 +256,38 @@ static int write_pid_file(void) pidfile = 0; return 1; } - (void)write(pidfd, val, (unsigned int)len); + if (write(pidfd, val, (unsigned int)len) != len) { + audit_msg(LOG_ERR, "Unable to write pidfile (%s)", + strerror(errno)); + close(pidfd); + pidfile = 0; + return 1; + } close(pidfd); return 0; } static void avoid_oom_killer(void) { - int oomfd; + int oomfd, len, rc; + char *score = NULL; /* New kernels use different technique */ - oomfd = open("/proc/self/oom_score_adj", O_NOFOLLOW | O_WRONLY); - if (oomfd >= 0) { - (void)write(oomfd, "-1000", 5); - close(oomfd); - return; - } - oomfd = open("/proc/self/oom_adj", O_NOFOLLOW | O_WRONLY); - if (oomfd >= 0) { - (void)write(oomfd, "-17", 3); - close(oomfd); - return; - } - // Old style kernel...perform another action here + if ((oomfd = open("/proc/self/oom_score_adj", + O_NOFOLLOW | O_WRONLY)) >= 0) { + score = "-1000"; + } else if ((oomfd = open("/proc/self/oom_adj", + O_NOFOLLOW | O_WRONLY)) >= 0) { + score = "-17"; + } else + audit_msg(LOG_NOTICE, "Cannot open out of memory adjuster"); + + len = strlen(score); + rc = write(oomfd, score, len); + if (rc != len) + audit_msg(LOG_NOTICE, "Unable to adjust out of memory score"); + + close(oomfd); } /* @@ -328,7 +337,12 @@ static int become_daemon(void) close(fd); /* Change to '/' */ - chdir("/"); + rc = chdir("/"); + if (rc < 0) { + audit_msg(LOG_ERR, + "Cannot change working directory to /"); + return -1; + } /* Become session/process group leader */ setsid(); @@ -540,8 +554,8 @@ int main(int argc, char *argv[]) if (config.priority_boost != 0) { errno = 0; - (void) nice((int)-config.priority_boost); - if (errno) { + rc = nice((int)-config.priority_boost); + if (rc == -1 && errno) { audit_msg(LOG_ERR, "Cannot change priority (%s)", strerror(errno)); return 1; diff --git a/src/autrace.c b/src/autrace.c index e124660..47c6c4f 100644 --- a/src/autrace.c +++ b/src/autrace.c @@ -245,7 +245,11 @@ int main(int argc, char *argv[]) exit(1); } sleep(1); - (void)write(fd[1],"1", 1); + if (write(fd[1],"1", 1) != 1) { + kill(pid,SIGTERM); + (void)delete_all_rules(audit_fd); + exit(1); + } waitpid(pid, NULL, 0); close(fd[1]); puts("Cleaning up..."); -- 1.7.10.4