From 78751e0fc9ebc688d4db920866edd4e53cb1f5e9 Mon Sep 17 00:00:00 2001 From: Erik Karlsson Date: Fri, 1 Apr 2022 19:54:51 +0200 Subject: [PATCH] dmcommon: remove unused pipe in dmcmd This avoids leaking file descriptors and potential deadlock in case the command fills the pipe. Output is sent to /dev/null and dmcmd will return the exit status instead of a file descriptor. --- dmdiagnostics.c | 2 +- dmtree/tr181/ip.c | 2 +- dmtree/tr181/times.c | 8 +-- dmtree/tr181/wifi.c | 2 +- libbbf_api/dmcommon.c | 108 +++++++++++++++++------------------ libbbf_api/dmcommon.h | 20 +++---- test/cmocka/unit_test_bbfd.c | 4 +- 7 files changed, 69 insertions(+), 77 deletions(-) diff --git a/dmdiagnostics.c b/dmdiagnostics.c index a6f95c5d..9ade44a7 100644 --- a/dmdiagnostics.c +++ b/dmdiagnostics.c @@ -61,7 +61,7 @@ void init_diagnostics_operation(char *sec_name, char *operation_path) if (section) dmuci_delete_by_section_bbfdm(section, NULL, NULL); - DMCMD("/bin/sh", 2, operation_path, "stop"); + dmcmd("/bin/sh", 2, operation_path, "stop"); } void set_diagnostics_interface_option(struct dmctx *ctx, char *sec_name, char *value) diff --git a/dmtree/tr181/ip.c b/dmtree/tr181/ip.c index 777578cc..49b4e7a9 100644 --- a/dmtree/tr181/ip.c +++ b/dmtree/tr181/ip.c @@ -1115,7 +1115,7 @@ static int set_IP_IPv6Enable(char *refparam, struct dmctx *ctx, void *data, char return 0; snprintf(buf, sizeof(buf), "net.ipv6.conf.all.disable_ipv6=%d", b ? 0 : 1); - DMCMD("sysctl", 2, "-w", buf); + dmcmd("sysctl", 2, "-w", buf); fseek(fp, 0, SEEK_END); long length = ftell(fp); diff --git a/dmtree/tr181/times.c b/dmtree/tr181/times.c index dff2b6db..1065ec1d 100644 --- a/dmtree/tr181/times.c +++ b/dmtree/tr181/times.c @@ -38,16 +38,16 @@ static int set_time_enable(char *refparam, struct dmctx *ctx, void *data, char * case VALUESET: string_to_bool(value, &b); if(b) { - DMCMD("/etc/rc.common", 2, "/etc/init.d/ntpd", "enable"); + dmcmd("/etc/rc.common", 2, "/etc/init.d/ntpd", "enable"); pid = get_pid("ntpd"); if (pid < 0) { - DMCMD("/etc/rc.common", 2, "/etc/init.d/ntpd", "start"); + dmcmd("/etc/rc.common", 2, "/etc/init.d/ntpd", "start"); } } else { - DMCMD("/etc/rc.common", 2, "/etc/init.d/ntpd", "disable"); + dmcmd("/etc/rc.common", 2, "/etc/init.d/ntpd", "disable"); pid = get_pid("ntpd"); if (pid > 0) { - DMCMD("/etc/rc.common", 2, "/etc/init.d/ntpd", "stop"); + dmcmd("/etc/rc.common", 2, "/etc/init.d/ntpd", "stop"); } } return 0; diff --git a/dmtree/tr181/wifi.c b/dmtree/tr181/wifi.c index 20f06ae8..c1c611e7 100644 --- a/dmtree/tr181/wifi.c +++ b/dmtree/tr181/wifi.c @@ -5642,7 +5642,7 @@ static int get_WiFiDataElementsDisassociationEventDisassociationEventData_TimeSt *************************************************************/ static int operate_WiFi_Reset(char *refparam, struct dmctx *ctx, void *data, char *instance, char *value, int action) { - return !dmcmd_no_wait("/sbin/wifi", 2, "reload", "&") ? CMD_SUCCESS : CMD_FAIL; + return !dmcmd_no_wait("/sbin/wifi", 1, "reload") ? CMD_SUCCESS : CMD_FAIL; } static operation_args neighboring_wifi_diagnostic_args = { diff --git a/libbbf_api/dmcommon.c b/libbbf_api/dmcommon.c index 8850fc5b..0d61915b 100644 --- a/libbbf_api/dmcommon.c +++ b/libbbf_api/dmcommon.c @@ -141,81 +141,79 @@ void remove_new_line(char *buf) buf[len - 1] = 0; } -int dmcmd(char *cmd, int n, ...) +static void dmcmd_exec(char *argv[]) { - va_list arg; - int i, pid; - int dmcmd_pfds[2]; - char *argv[n+2]; + int devnull = open("/dev/null", O_RDWR); - argv[0] = cmd; + if (devnull == -1) + exit(127); - va_start(arg,n); - for (i=0; i 2) + close(devnull); - if (pipe(dmcmd_pfds) < 0) - return -1; - - if ((pid = fork()) == -1) - return -1; - - if (pid == 0) { - /* child */ - close(dmcmd_pfds[0]); - dup2(dmcmd_pfds[1], 1); - close(dmcmd_pfds[1]); - - execvp(argv[0], (char **) argv); /* Flawfinder: ignore */ - exit(ESRCH); - } else if (pid < 0) - return -1; - - /* parent */ - close(dmcmd_pfds[1]); - - int status; - while (waitpid(pid, &status, 0) != pid) - { - kill(pid, 0); - if (errno == ESRCH) { - return dmcmd_pfds[0]; - } - } - - return dmcmd_pfds[0]; + execvp(argv[0], argv); /* Flawfinder: ignore */ + exit(127); } -int dmcmd_no_wait(char *cmd, int n, ...) +int dmcmd(char *cmd, int n, ...) { + char *argv[n + 2]; va_list arg; - int i, pid; - char *argv[n+2]; - char sargv[4][128]; + int i, status; + pid_t pid, wpid; argv[0] = cmd; va_start(arg, n); for (i = 0; i < n; i++) { - DM_STRNCPY(sargv[i], va_arg(arg, char*), sizeof(sargv[i])); - argv[i+1] = sargv[i]; + argv[i + 1] = va_arg(arg, char *); } va_end(arg); - - argv[n+1] = NULL; + argv[n + 1] = NULL; if ((pid = fork()) == -1) return -1; - if (pid == 0) { - execvp(argv[0], (char **) argv); /* Flawfinder: ignore */ - exit(ESRCH); - } else if (pid < 0) + if (pid == 0) + dmcmd_exec(argv); + + do { + wpid = waitpid(pid, &status, 0); + if (wpid == pid) { + if (WIFEXITED(status)) + return WEXITSTATUS(status); + if (WIFSIGNALED(status)) + return 128 + WTERMSIG(status); + } + } while (wpid == -1 && errno == EINTR); + + return -1; +} + +int dmcmd_no_wait(char *cmd, int n, ...) +{ + char *argv[n + 2]; + va_list arg; + int i; + pid_t pid; + + argv[0] = cmd; + va_start(arg, n); + for (i = 0; i < n; i++) { + argv[i + 1] = va_arg(arg, char *); + } + va_end(arg); + argv[n + 1] = NULL; + + if ((pid = fork()) == -1) return -1; + + if (pid == 0) + dmcmd_exec(argv); + return 0; } diff --git a/libbbf_api/dmcommon.h b/libbbf_api/dmcommon.h index 6454bbed..f9958d53 100644 --- a/libbbf_api/dmcommon.h +++ b/libbbf_api/dmcommon.h @@ -159,12 +159,6 @@ do { \ *dest = '\0'; \ } while(0) -#define DMCMD(CMD, N, ...) \ -do { \ - int mpp = dmcmd(CMD, N, ## __VA_ARGS__); \ - if (mpp) close (mpp); \ -} while (0) - enum fs_size_type_enum { FS_SIZE_TOTAL, FS_SIZE_AVAILABLE, @@ -172,22 +166,22 @@ enum fs_size_type_enum { }; #define IPPING_PATH "/usr/share/bbfdm/ipping_launch" -#define IPPING_STOP DMCMD("/bin/sh", 2, IPPING_PATH, "stop"); +#define IPPING_STOP dmcmd("/bin/sh", 2, IPPING_PATH, "stop"); #define DOWNLOAD_DIAGNOSTIC_PATH "/usr/share/bbfdm/download_launch" #define DOWNLOAD_DUMP_FILE "/tmp/download_dump" -#define DOWNLOAD_DIAGNOSTIC_STOP DMCMD("/bin/sh", 2, DOWNLOAD_DIAGNOSTIC_PATH, "stop"); +#define DOWNLOAD_DIAGNOSTIC_STOP dmcmd("/bin/sh", 2, DOWNLOAD_DIAGNOSTIC_PATH, "stop"); #define UPLOAD_DIAGNOSTIC_PATH "/usr/share/bbfdm/upload_launch" #define UPLOAD_DUMP_FILE "/tmp/upload_dump" -#define UPLOAD_DIAGNOSTIC_STOP DMCMD("/bin/sh", 2, UPLOAD_DIAGNOSTIC_PATH, "stop"); +#define UPLOAD_DIAGNOSTIC_STOP dmcmd("/bin/sh", 2, UPLOAD_DIAGNOSTIC_PATH, "stop"); #define NSLOOKUP_PATH "/usr/share/bbfdm/nslookup_launch" #define NSLOOKUP_LOG_FILE "/tmp/nslookup.log" -#define NSLOOKUP_STOP DMCMD("/bin/sh", 2, NSLOOKUP_PATH, "stop"); +#define NSLOOKUP_STOP dmcmd("/bin/sh", 2, NSLOOKUP_PATH, "stop"); #define TRACEROUTE_PATH "/usr/share/bbfdm/traceroute_launch" -#define TRACEROUTE_STOP DMCMD("/bin/sh", 2, TRACEROUTE_PATH, "stop"); +#define TRACEROUTE_STOP dmcmd("/bin/sh", 2, TRACEROUTE_PATH, "stop"); #define UDPECHO_PATH "/usr/share/bbfdm/udpecho_launch" -#define UDPECHO_STOP DMCMD("/bin/sh", 2, UDPECHO_PATH, "stop"); +#define UDPECHO_STOP dmcmd("/bin/sh", 2, UDPECHO_PATH, "stop"); #define SERVERSELECTION_PATH "/usr/share/bbfdm/serverselection_launch" -#define SERVERSELECTION_STOP DMCMD("/bin/sh", 2, SERVERSELECTION_PATH, "stop"); +#define SERVERSELECTION_STOP dmcmd("/bin/sh", 2, SERVERSELECTION_PATH, "stop"); #define sysfs_foreach_file(path,dir,ent) \ if ((dir = opendir(path)) == NULL) return 0; \ diff --git a/test/cmocka/unit_test_bbfd.c b/test/cmocka/unit_test_bbfd.c index f16af7fd..a2169375 100644 --- a/test/cmocka/unit_test_bbfd.c +++ b/test/cmocka/unit_test_bbfd.c @@ -604,7 +604,7 @@ static void test_api_bbfdm_json_set_value(void **state) struct param_fault *first_fault; int fault = 0; - DMCMD("/bin/cp", 2, DROPBEAR_FILE_PATH, DROPBEAR_JSON_PATH); + dmcmd("/bin/cp", 2, DROPBEAR_FILE_PATH, DROPBEAR_JSON_PATH); fault = dm_entry_param_method(ctx, CMD_SET_VALUE, "Device.UserInterface.Enable", "true", NULL); assert_int_equal(fault, 0); @@ -688,7 +688,7 @@ static void test_api_bbfdm_library_set_value(void **state) struct param_fault *first_fault; int fault = 0; - DMCMD("/bin/cp", 2, LIBBBF_TEST_PATH, LIBBBF_TEST_BBFDM_PATH); + dmcmd("/bin/cp", 2, LIBBBF_TEST_PATH, LIBBBF_TEST_BBFDM_PATH); fault = dm_entry_param_method(ctx, CMD_SET_VALUE, "Device.ManagementServer.EnableCWMP", "true", NULL); assert_int_equal(fault, 0);