From 29f58815b63cd84523f4d4c5438da17dfc560959 Mon Sep 17 00:00:00 2001 From: Kevin Day Date: Tue, 7 Jun 2022 23:38:07 -0500 Subject: [PATCH] Bugfix: Problems exposed by unit tests for f_signal. Fix ordering of functions. Compare using "== -1" rather than "< 0" because the POSIX standard says "-1" rather than "less than zero". The f_signal_read() function has several problems: - Should be passing a pointer rather than an array to poll(). - Needs to check data_poll.revents. - Missing the parameter checking. --- level_0/f_signal/c/signal.c | 142 +++++++++++++++++++---------------- level_0/f_signal/c/signal.h | 111 ++++++++++++++------------- level_0/f_signal/c/signal/common.h | 2 +- level_0/f_signal/data/build/settings | 2 +- 4 files changed, 136 insertions(+), 121 deletions(-) diff --git a/level_0/f_signal/c/signal.c b/level_0/f_signal/c/signal.c index 54ae284..664d4b3 100644 --- a/level_0/f_signal/c/signal.c +++ b/level_0/f_signal/c/signal.c @@ -14,7 +14,7 @@ extern "C" { return F_data_not; } - if (close(signal->id) < 0) { + if (close(signal->id) == -1) { if (errno == EBADF) return F_status_set_error(F_descriptor); if (errno == EDQUOT) return F_status_set_error(F_filesystem_quota_block); if (errno == EINTR) return F_status_set_error(F_interrupt); @@ -30,6 +30,23 @@ extern "C" { } #endif // _di_f_signal_close_ +#ifndef _di_f_signal_mask_ + f_status_t f_signal_mask(const int how, const sigset_t * const next, sigset_t * const current) { + #ifndef _di_level_0_parameter_checking_ + if (!next && !current) return F_status_set_error(F_parameter); + #endif // _di_level_0_parameter_checking_ + + if (sigprocmask(how, next, current) == -1) { + if (errno == EFAULT) return F_status_set_error(F_buffer); + if (errno == EINVAL) return F_status_set_error(F_parameter); + + return F_status_set_error(F_failure); + } + + return F_none; + } +#endif // _di_f_signal_mask_ + #ifndef _di_f_signal_open_ f_status_t f_signal_open(f_signal_t * const signal) { #ifndef _di_level_0_parameter_checking_ @@ -38,7 +55,7 @@ extern "C" { const int result = signalfd(signal->id, &signal->set, signal->flags); - if (result < 0) { + if (result == -1) { if (errno == EINVAL) return F_status_set_error(F_parameter); if (errno == EMFILE) return F_status_set_error(F_file_descriptor_max); if (errno == ENFILE) return F_status_set_error(F_file_open_max); @@ -54,24 +71,37 @@ extern "C" { } #endif // _di_f_signal_open_ -#ifndef _di_f_signal_read_ - f_status_t f_signal_read(const f_signal_t signal, const int timeout, struct signalfd_siginfo * const information) { +#ifndef _di_f_signal_queue_ + f_status_t f_signal_queue(const pid_t id, const int signal, const union sigval value) { - if (!signal.id) { - return F_data_not; + if (sigqueue(id, signal, value) == -1) { + if (errno == EAGAIN) return F_status_set_error(F_resource_not); + if (errno == ENOSYS) return F_status_set_error(F_supported_not); + if (errno == EINVAL) return F_status_set_error(F_parameter); + if (errno == ESRCH) return F_status_set_error(F_found_not); + + return F_status_set_error(F_failure); } + return F_none; + } +#endif // _di_f_signal_queue_ + +#ifndef _di_f_signal_read_ + f_status_t f_signal_read(const f_signal_t signal, const int timeout, struct signalfd_siginfo * const information) { + #ifndef _di_level_0_parameter_checking_ + if (!information) return F_status_set_error(F_parameter); + #endif // _di_level_0_parameter_checking_ + struct pollfd data_poll; memset(&data_poll, 0, sizeof(struct pollfd)); data_poll.fd = signal.id; data_poll.events = POLLIN; - struct pollfd polls[] = { data_poll }; - - const int result = poll(polls, 1, timeout); + const int result = poll(&data_poll, 1, timeout); - if (result < 0) { + if (result == -1) { if (errno == EFAULT) return F_status_set_error(F_buffer); if (errno == EINTR) return F_status_set_error(F_interrupt); if (errno == EINVAL) return F_status_set_error(F_parameter); @@ -79,22 +109,37 @@ extern "C" { return F_status_set_error(F_failure); } - else if (result) { - const ssize_t total = read(signal.id, information, sizeof(struct signalfd_siginfo)); - - if (total < 0) { - if (errno == EAGAIN || errno == EWOULDBLOCK) return F_status_set_error(F_block); - if (errno == EBADF) return F_status_set_error(F_descriptor); - if (errno == EFAULT) return F_status_set_error(F_buffer); - if (errno == EINTR) return F_status_set_error(F_interrupt); - if (errno == EINVAL) return F_status_set_error(F_parameter); - if (errno == EIO) return F_status_set_error(F_input_output); - if (errno == EISDIR) return F_status_set_error(F_file_type_directory); - - return F_status_set_error(F_failure); + + if (result) { + if (data_poll.revents & POLLNVAL) { + return F_status_set_error(F_parameter); } - return F_signal; + if (data_poll.revents & POLLHUP) { + return F_status_set_error(F_file_closed); + } + + if (data_poll.revents & POLLERR) { + return F_status_set_error(F_stream); + } + + if (data_poll.revents & POLLIN) { + const ssize_t total = read(signal.id, information, sizeof(struct signalfd_siginfo)); + + if (total == -1) { + if (errno == EAGAIN || errno == EWOULDBLOCK) return F_status_set_error(F_block); + if (errno == EBADF) return F_status_set_error(F_descriptor); + if (errno == EFAULT) return F_status_set_error(F_buffer); + if (errno == EINTR) return F_status_set_error(F_interrupt); + if (errno == EINVAL) return F_status_set_error(F_parameter); + if (errno == EIO) return F_status_set_error(F_input_output); + if (errno == EISDIR) return F_status_set_error(F_file_type_directory); + + return F_status_set_error(F_failure); + } + + return F_signal; + } } return F_none; @@ -104,7 +149,7 @@ extern "C" { #ifndef _di_f_signal_send_ f_status_t f_signal_send(const int signal, const pid_t process_id) { - if (kill(process_id, signal) < 0) { + if (kill(process_id, signal) == -1) { if (errno == EINVAL) return F_status_set_error(F_parameter); if (errno == EPERM) return F_status_set_error(F_prohibited); if (errno == ESRCH) return F_status_set_error(F_found_not); @@ -122,7 +167,7 @@ extern "C" { if (!set) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ - if (sigaddset(set, signal) < 0) { + if (sigaddset(set, signal) == -1) { if (errno == EINVAL) return F_status_set_error(F_parameter); return F_status_set_error(F_failure); @@ -138,7 +183,7 @@ extern "C" { if (!set) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ - if (sigdelset(set, signal) < 0) { + if (sigdelset(set, signal) == -1) { if (errno == EINVAL) return F_status_set_error(F_parameter); return F_status_set_error(F_failure); @@ -154,7 +199,7 @@ extern "C" { if (!set) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ - if (sigemptyset(set) < 0) { + if (sigemptyset(set) == -1) { if (errno == EINVAL) return F_status_set_error(F_parameter); return F_status_set_error(F_failure); @@ -170,7 +215,7 @@ extern "C" { if (!set) return F_status_set_error(F_parameter); #endif // _di_level_0_parameter_checking_ - if (sigfillset(set) < 0) { + if (sigfillset(set) == -1) { if (errno == EFAULT) return F_status_set_error(F_buffer); if (errno == EINVAL) return F_status_set_error(F_parameter); @@ -181,39 +226,6 @@ extern "C" { } #endif // _di_f_signal_set_fill_ -#ifndef _di_f_signal_mask_ - f_status_t f_signal_mask(const int how, const sigset_t * const next, sigset_t * const current) { - #ifndef _di_level_0_parameter_checking_ - if (!next && !current) return F_status_set_error(F_parameter); - #endif // _di_level_0_parameter_checking_ - - if (sigprocmask(how, next, current) < 0) { - if (errno == EFAULT) return F_status_set_error(F_buffer); - if (errno == EINVAL) return F_status_set_error(F_parameter); - - return F_status_set_error(F_failure); - } - - return F_none; - } -#endif // _di_f_signal_mask_ - -#ifndef _di_f_signal_queue_ - f_status_t f_signal_queue(const pid_t id, const int signal, const union sigval value) { - - if (sigqueue(id, signal, value) < 0) { - if (errno == EAGAIN) return F_status_set_error(F_resource_not); - if (errno == ENOSYS) return F_status_set_error(F_supported_not); - if (errno == EINVAL) return F_status_set_error(F_parameter); - if (errno == ESRCH) return F_status_set_error(F_found_not); - - return F_status_set_error(F_failure); - } - - return F_none; - } -#endif // _di_f_signal_queue_ - #ifndef _di_f_signal_set_has_ f_status_t f_signal_set_has(const int signal, const sigset_t * const set) { #ifndef _di_level_0_parameter_checking_ @@ -222,7 +234,7 @@ extern "C" { const int result = sigismember(set, signal); - if (result < 0) { + if (result == -1) { if (errno == EINVAL) return F_status_set_error(F_parameter); return F_status_set_error(F_failure); @@ -235,7 +247,7 @@ extern "C" { #ifndef _di_f_signal_wait_ f_status_t f_signal_wait(const sigset_t * const set, siginfo_t * const information) { - if (sigwaitinfo(set, information) < 0) { + if (sigwaitinfo(set, information) == -1) { if (errno == EINTR) return F_status_set_error(F_interrupt); if (errno == EINVAL) return F_status_set_error(F_parameter); @@ -249,7 +261,7 @@ extern "C" { #ifndef _di_f_signal_wait_until_ f_status_t f_signal_wait_until(const sigset_t * const set, const struct timespec * timeout, siginfo_t * const information) { - if (sigtimedwait(set, information, timeout) < 0) { + if (sigtimedwait(set, information, timeout) == -1) { if (errno == EAGAIN) return F_time_out; if (errno == EINTR) return F_status_set_error(F_interrupt); if (errno == EINVAL) return F_status_set_error(F_parameter); diff --git a/level_0/f_signal/c/signal.h b/level_0/f_signal/c/signal.h index 1f98a8d..3d811f6 100644 --- a/level_0/f_signal/c/signal.h +++ b/level_0/f_signal/c/signal.h @@ -54,6 +54,34 @@ extern "C" { #endif // _di_f_signal_close_ /** + * Get or assign the current signal set in use. + * + * Either set or previous may be NULL but not both (at least one is required). + * + * @param how + * How to handle the signal. + * Set this to 0 when only trying to get the current signal set. + * @param next + * (optional) The new set of signals to handle. + * Set to NULL to not use. + * @param current + * (optional) The current set of signals being handled. + * Set to NULL to not use. + * + * @return + * F_none on success but no signal found. + * + * F_parameter (with error bit) if a parameter is invalid. + * + * F_failure (with error bit) for any other error. + * + * @see sigprocmask() + */ +#ifndef _di_f_signal_mask_ + extern f_status_t f_signal_mask(const int how, const sigset_t * const next, sigset_t * const current); +#endif // _di_f_signal_mask_ + +/** * Open a signal descriptor, listening for the given set of signals. * * The signal.id is assigned with the signal descriptor on success. @@ -66,6 +94,7 @@ extern "C" { * * @return * F_none on success but no signal found. + * * F_device (with error bit) if could not mount the internal inode device. * F_file_descriptor_max (with error bit) if max file descriptors is reached. * F_memory_not (with error bit) if out of memory. @@ -80,6 +109,32 @@ extern "C" { #endif // _di_f_signal_open_ /** + * Send the signal and value to the given process. + * + * @param id + * The PID to signal. + * @param signal + * The signal to send to the thread. + * @param value + * The signal value to send. + * + * @return + * F_none on success but no signal found. + * + * F_found_not (with error bit) if the given PID was found. + * F_parameter (with error bit) if a parameter is invalid. + * F_resource_not (with error bit) if the max signals is reached. + * F_supported_not (with error bit) if this action is not supported by the current OS. + * + * F_failure (with error bit) for any other error. + * + * @see sigqueue() + */ +#ifndef _di_f_signal_queue_ + extern f_status_t f_signal_queue(const pid_t id, const int signal, const union sigval value); +#endif // _di_f_signal_queue_ + +/** * Read a current process signal, if one exists. * * @param signal @@ -98,9 +153,11 @@ extern "C" { * F_block (with error bit) if file descriptor is set to non-block and the read would result in a blocking operation. * F_buffer (with error bit) if the buffer is invalid. * F_descriptor (with error bit) if the signal descriptor is invalid. + * F_file_closed (with error bit) if the signal descriptor stream is closed. * F_interrupt (with error bit) if interrupt was received. * F_memory_not (with error bit) on out of memory. * F_parameter (with error bit) if a parameter is invalid. + * F_stream (with error bit) when the poll() returns POLLERR (file stream error). * F_input_output (with error bit) on I/O error. * F_file_type_directory (with error bit) if file descriptor represents a directory. * @@ -218,60 +275,6 @@ extern "C" { #endif // _di_f_signal_set_fill_ /** - * Get or assign the current signal set in use. - * - * Either set or previous may be NULL but not both (at least one is required). - * - * @param how - * How to handle the signal. - * Set this to 0 when only trying to get the current signal set. - * @param next - * (optional) The new set of signals to handle. - * Set to NULL to not use. - * @param current - * (optional) The current set of signals being handled. - * Set to NULL to not use. - * - * @return - * F_none on success but no signal found. - * - * F_parameter (with error bit) if a parameter is invalid. - * - * F_failure (with error bit) for any other error. - * - * @see sigprocmask() - */ -#ifndef _di_f_signal_mask_ - extern f_status_t f_signal_mask(const int how, const sigset_t * const next, sigset_t * const current); -#endif // _di_f_signal_mask_ - -/** - * Send the signal and value to the given process. - * - * @param id - * The PID to signal. - * @param signal - * The signal to send to the thread. - * @param value - * The signal value to send. - * - * @return - * F_none on success but no signal found. - * - * F_found_not (with error bit) if the given PID was found. - * F_parameter (with error bit) if a parameter is invalid. - * F_resource_not (with error bit) if the max signals is reached. - * F_supported_not (with error bit) if this action is not supported by the current OS. - * - * F_failure (with error bit) for any other error. - * - * @see sigqueue() - */ -#ifndef _di_f_signal_queue_ - extern f_status_t f_signal_queue(const pid_t id, const int signal, const union sigval value); -#endif // _di_f_signal_queue_ - -/** * Check to see if the given signal set has a given signal. * * @param signal diff --git a/level_0/f_signal/c/signal/common.h b/level_0/f_signal/c/signal/common.h index 8d3d67d..a61742a 100644 --- a/level_0/f_signal/c/signal/common.h +++ b/level_0/f_signal/c/signal/common.h @@ -30,7 +30,7 @@ extern "C" { sigset_t set; } f_signal_t; - #define f_signal_t_initialize {0, -1, { 0 } } + #define f_signal_t_initialize { 0, -1, { 0 } } #define macro_f_signal_t_initialize(flags, id, set) { flags, id, set } diff --git a/level_0/f_signal/data/build/settings b/level_0/f_signal/data/build/settings index f10f330..8765d33 100644 --- a/level_0/f_signal/data/build/settings +++ b/level_0/f_signal/data/build/settings @@ -47,7 +47,7 @@ search_static yes flags -O2 -z now -g -fdiagnostics-color=always -Wno-logical-not-parentheses -Wno-parentheses flags-clang -Wno-logical-op-parentheses -flags-test -O0 -fstack-protector -Wall +flags-test -O0 -fstack-protector -Wall -Wno-missing-braces flags-coverage -O0 --coverage -fprofile-abs-path -fprofile-dir=build/coverage/ flags_library -fPIC -- 1.8.3.1