[pacman-dev] [PATCH 1/2] [WIP] move wordsplit into ini for sharing
Not technically related to INI parsing, but we use it with INI files. --- lib/libalpm/hook.c | 113 --------------------------------------------- src/common/ini.c | 113 +++++++++++++++++++++++++++++++++++++++++++++ src/common/ini.h | 3 ++ 3 files changed, 116 insertions(+), 113 deletions(-) diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index 6143ea0f..c385b5d5 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -17,7 +17,6 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ -#include <ctype.h> #include <dirent.h> #include <errno.h> #include <limits.h> @@ -71,17 +70,6 @@ static void _alpm_trigger_free(struct _alpm_trigger_t *trigger) } } -static void _alpm_wordsplit_free(char **ws) -{ - if(ws) { - char **c; - for(c = ws; *c; c++) { - free(*c); - } - free(ws); - } -} - static void _alpm_hook_free(struct _alpm_hook_t *hook) { if(hook) { @@ -158,107 +146,6 @@ static int _alpm_hook_validate(alpm_handle_t *handle, return ret; } -static char **_alpm_wordsplit(char *str) -{ - char *c = str, *end; - char **out = NULL, **outsave; - size_t count = 0; - - if(str == NULL) { - errno = EINVAL; - return NULL; - } - - for(c = str; isspace(*c); c++); - while(*c) { - size_t wordlen = 0; - - /* extend our array */ - outsave = out; - if((out = realloc(out, (count + 1) * sizeof(char*))) == NULL) { - out = outsave; - goto error; - } - - /* calculate word length and check for unbalanced quotes */ - for(end = c; *end && !isspace(*end); end++) { - if(*end == '\'' || *end == '"') { - char quote = *end; - while(*(++end) && *end != quote) { - if(*end == '\\' && *(end + 1) == quote) { - end++; - } - wordlen++; - } - if(*end != quote) { - errno = EINVAL; - goto error; - } - } else { - if(*end == '\\' && (end[1] == '\'' || end[1] == '"')) { - end++; /* skip the '\\' */ - } - wordlen++; - } - } - - if(wordlen == (size_t) (end - c)) { - /* no internal quotes or escapes, copy it the easy way */ - if((out[count++] = strndup(c, wordlen)) == NULL) { - goto error; - } - } else { - /* manually copy to remove quotes and escapes */ - char *dest = out[count++] = malloc(wordlen + 1); - if(dest == NULL) { goto error; } - while(c < end) { - if(*c == '\'' || *c == '"') { - char quote = *c; - /* we know there must be a matching end quote, - * no need to check for '\0' */ - for(c++; *c != quote; c++) { - if(*c == '\\' && *(c + 1) == quote) { - c++; - } - *(dest++) = *c; - } - c++; - } else { - if(*c == '\\' && (c[1] == '\'' || c[1] == '"')) { - c++; /* skip the '\\' */ - } - *(dest++) = *(c++); - } - } - *dest = '\0'; - } - - if(*end == '\0') { - break; - } else { - for(c = end + 1; isspace(*c); c++); - } - } - - outsave = out; - if((out = realloc(out, (count + 1) * sizeof(char*))) == NULL) { - out = outsave; - goto error; - } - - out[count++] = NULL; - - return out; - -error: - /* can't use wordsplit_free here because NULL has not been appended */ - while(count) { - free(out[--count]); - } - free(out); - return NULL; -} - static int _alpm_hook_parse_cb(const char *file, int line, const char *section, char *key, char *value, void *data) { diff --git a/src/common/ini.c b/src/common/ini.c index 30f1fac8..9cdd5c47 100644 --- a/src/common/ini.c +++ b/src/common/ini.c @@ -17,6 +17,7 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ +#include <ctype.h> #include <errno.h> #include <limits.h> #include <stdlib.h> @@ -106,3 +107,115 @@ cleanup: free(section_name); return ret; } + +void _alpm_wordsplit_free(char **ws) +{ + if(ws) { + char **c; + for(c = ws; *c; c++) { + free(*c); + } + free(ws); + } +} + +char **_alpm_wordsplit(const char *str) +{ + const char *c = str, *end; + char **out = NULL, **outsave; + size_t count = 0; + + if(str == NULL) { + errno = EINVAL; + return NULL; + } + + for(c = str; isspace(*c); c++); + while(*c) { + size_t wordlen = 0; + + /* extend our array */ + outsave = out; + if((out = realloc(out, (count + 1) * sizeof(char*))) == NULL) { + out = outsave; + goto error; + } + + /* calculate word length and check for unbalanced quotes */ + for(end = c; *end && !isspace(*end); end++) { + if(*end == '\'' || *end == '"') { + char quote = *end; + while(*(++end) && *end != quote) { + if(*end == '\\' && *(end + 1) == quote) { + end++; + } + wordlen++; + } + if(*end != quote) { + errno = EINVAL; + goto error; + } + } else { + if(*end == '\\' && (end[1] == '\'' || end[1] == '"')) { + end++; /* skip the '\\' */ + } + wordlen++; + } + } + + if(wordlen == (size_t) (end - c)) { + /* no internal quotes or escapes, copy it the easy way */ + if((out[count++] = strndup(c, wordlen)) == NULL) { + goto error; + } + } else { + /* manually copy to remove quotes and escapes */ + char *dest = out[count++] = malloc(wordlen + 1); + if(dest == NULL) { goto error; } + while(c < end) { + if(*c == '\'' || *c == '"') { + char quote = *c; + /* we know there must be a matching end quote, + * no need to check for '\0' */ + for(c++; *c != quote; c++) { + if(*c == '\\' && *(c + 1) == quote) { + c++; + } + *(dest++) = *c; + } + c++; + } else { + if(*c == '\\' && (c[1] == '\'' || c[1] == '"')) { + c++; /* skip the '\\' */ + } + *(dest++) = *(c++); + } + } + *dest = '\0'; + } + + if(*end == '\0') { + break; + } else { + for(c = end + 1; isspace(*c); c++); + } + } + + outsave = out; + if((out = realloc(out, (count + 1) * sizeof(char*))) == NULL) { + out = outsave; + goto error; + } + + out[count++] = NULL; + + return out; + +error: + /* can't use wordsplit_free here because NULL has not been appended */ + while(count) { + free(out[--count]); + } + free(out); + return NULL; +} diff --git a/src/common/ini.h b/src/common/ini.h index 6d2b264a..b7c40f2c 100644 --- a/src/common/ini.h +++ b/src/common/ini.h @@ -25,4 +25,7 @@ typedef int (ini_parser_fn)(const char *file, int line, const char *section, int parse_ini(const char *file, ini_parser_fn cb, void *data); +void _alpm_wordsplit_free(char **ws); +char **_alpm_wordsplit(const char *str); + #endif /* PM_INI_H */ -- 2.21.0
--- systemvp should pretty much be a drop-in replacement for system with the exception that it takes an argv array and uses exec. If anybody wants to play with it to stress test it a little, I have a self-contained copy and test program at: https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c TODO: * update docs * fix debug logging * should the command be run with PATH lookup (execv vs execvp)? * Is the use of mmap with MAP_ANONYMOUS okay? MAP_ANONYMOUS is not POSIX but "most systems also support MAP_ANONYMOUS (or its synonym MAP_ANON)" (mmap(2)). * should we reset signals prior to exec'ing like we do with hooks/scripts? src/pacman/conf.c | 95 ++++++++++++++++++++++++++++++++++++++--------- src/pacman/conf.h | 2 + 2 files changed, 80 insertions(+), 17 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 2d8518c4..faf446dc 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -26,9 +26,11 @@ #include <stdlib.h> #include <stdio.h> #include <string.h> /* strdup */ +#include <sys/mman.h> #include <sys/stat.h> #include <sys/types.h> #include <sys/utsname.h> /* uname */ +#include <sys/wait.h> #include <unistd.h> /* pacman */ @@ -153,6 +155,7 @@ int config_free(config_t *oldconfig) free(oldconfig->print_format); free(oldconfig->arch); free(oldconfig); + _alpm_wordsplit_free(oldconfig->xfercommand_argv); return 0; } @@ -201,18 +204,59 @@ static char *get_tempfile(const char *path, const char *filename) return tempfile; } +static int systemvp(const char *file, char *const argv[]) +{ + int pid, *err; + + err = mmap(NULL, sizeof(*err), PROT_READ | PROT_WRITE, + MAP_SHARED | MAP_ANONYMOUS, -1, 0); + if(err == NULL) { + return -1; + } + + pid = fork(); + if(pid == -1) { + /* error */ + munmap(err, sizeof(*err)); + return -1; + } else if(pid == 0) { + /* child */ + *err = 0; + execvp(file, argv); + *err = errno; + munmap(err, sizeof(*err)); + _Exit(1); + } else { + /* parent */ + int status; + while(waitpid(pid, &status, 0) == -1) { + if(errno != EINTR) { + munmap(err, sizeof(*err)); + return -1; + } + } + if(*err != 0) { + errno = *err; + status = -1; + } + munmap(err, sizeof(*err)); + return status; + } +} + /** External fetch callback */ static int download_with_xfercommand(const char *url, const char *localpath, int force) { int ret = 0, retval; int usepart = 0; - int cwdfd; + int cwdfd = -1; struct stat st; - char *parsedcmd, *tempcmd; char *destfile, *tempfile, *filename; + const char **argv; + size_t i; - if(!config->xfercommand) { + if(!config->xfercommand_argv) { return -1; } @@ -230,17 +274,20 @@ static int download_with_xfercommand(const char *url, const char *localpath, unlink(destfile); } - tempcmd = strdup(config->xfercommand); - /* replace all occurrences of %o with fn.part */ - if(strstr(tempcmd, "%o")) { - usepart = 1; - parsedcmd = strreplace(tempcmd, "%o", tempfile); - free(tempcmd); - tempcmd = parsedcmd; + if((argv = calloc(config->xfercommand_argc + 1, sizeof(char*))) == NULL) { + pm_printf(ALPM_LOG_ERROR, _("could not get current working directory\n")); + goto cleanup; + } + + for(i = 0; i <= config->xfercommand_argc; i++) { + const char *val = config->xfercommand_argv[i]; + if(val && strcmp(val, "%o") == 0) { + val = tempfile; + } else if(val && strcmp(val, "%u") == 0) { + val = url; + } + argv[i] = val; } - /* replace all occurrences of %u with the download URL */ - parsedcmd = strreplace(tempcmd, "%u", url); - free(tempcmd); /* save the cwd so we can restore it later */ do { @@ -256,9 +303,13 @@ static int download_with_xfercommand(const char *url, const char *localpath, ret = -1; goto cleanup; } - /* execute the parsed command via /bin/sh -c */ - pm_printf(ALPM_LOG_DEBUG, "running command: %s\n", parsedcmd); - retval = system(parsedcmd); + + printf("running command: %s", config->xfercommand_argv[0]); + for(i = 1; argv[i]; i++) { + printf(" '%s'", argv[i]); + } + printf("\n"); + retval = systemvp(argv[0], (char**)argv); if(retval == -1) { pm_printf(ALPM_LOG_WARNING, _("running XferCommand: fork failed!\n")); @@ -296,7 +347,6 @@ cleanup: } free(destfile); free(tempfile); - free(parsedcmd); return ret; } @@ -544,6 +594,17 @@ static int _parse_options(const char *key, char *value, pm_printf(ALPM_LOG_DEBUG, "config: logfile: %s\n", value); } } else if(strcmp(key, "XferCommand") == 0) { + char **c; + if((config->xfercommand_argv = _alpm_wordsplit(value)) == NULL) { + pm_printf(ALPM_LOG_WARNING, + _("config file %s, line %d: invalid value for '%s' : '%s'\n"), + file, linenum, "XferCommand", value); + return 1; + } + config->xfercommand_argc = 0; + for(c = config->xfercommand_argv; *c; c++) { + config->xfercommand_argc++; + } config->xfercommand = strdup(value); pm_printf(ALPM_LOG_DEBUG, "config: xfercommand: %s\n", value); } else if(strcmp(key, "CleanMethod") == 0) { diff --git a/src/pacman/conf.h b/src/pacman/conf.h index f45ed436..47df979d 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -125,6 +125,8 @@ typedef struct __config_t { alpm_list_t *noextract; alpm_list_t *overwrite_files; char *xfercommand; + char **xfercommand_argv; + size_t xfercommand_argc; /* our connection to libalpm */ alpm_handle_t *handle; -- 2.21.0
--- A few changes I omitted from the initial patch. Does anybody know what usepart was for? It was unset unless %o was used in XferCommand, but I'm not sure what the use case for an XferCommand without %o would be. src/pacman/conf.c | 11 ++++------- test/pacman/tests/sync200.py | 2 +- test/pacman/tests/xfercommand001.py | 2 +- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index faf446dc..c2b7da43 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -249,7 +249,6 @@ static int download_with_xfercommand(const char *url, const char *localpath, int force) { int ret = 0, retval; - int usepart = 0; int cwdfd = -1; struct stat st; char *destfile, *tempfile, *filename; @@ -322,12 +321,10 @@ static int download_with_xfercommand(const char *url, const char *localpath, } else { /* download was successful */ ret = 0; - if(usepart) { - if(rename(tempfile, destfile)) { - pm_printf(ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"), - tempfile, destfile, strerror(errno)); - ret = -1; - } + if(rename(tempfile, destfile)) { + pm_printf(ALPM_LOG_ERROR, _("could not rename %s to %s (%s)\n"), + tempfile, destfile, strerror(errno)); + ret = -1; } } diff --git a/test/pacman/tests/sync200.py b/test/pacman/tests/sync200.py index 2bcdd5d3..18f38b81 100644 --- a/test/pacman/tests/sync200.py +++ b/test/pacman/tests/sync200.py @@ -1,6 +1,6 @@ self.description = "Synchronize the local database" -self.option['XferCommand'] = ['/usr/bin/curl %u > %o'] +self.option['XferCommand'] = ['/usr/bin/curl %u -o %o'] sp1 = pmpkg("spkg1", "1.0-1") sp1.depends = ["spkg2"] diff --git a/test/pacman/tests/xfercommand001.py b/test/pacman/tests/xfercommand001.py index 0d244dc6..0ac99080 100644 --- a/test/pacman/tests/xfercommand001.py +++ b/test/pacman/tests/xfercommand001.py @@ -3,7 +3,7 @@ # this setting forces us to download packages self.cachepkgs = False #wget doesn't support file:// urls. curl does -self.option['XferCommand'] = ['/usr/bin/curl %u > %o'] +self.option['XferCommand'] = ['/usr/bin/curl %u -o %o'] numpkgs = 10 pkgnames = [] -- 2.21.0
On 10/6/19 6:50 am, Andrew Gregory wrote:
Does anybody know what usepart was for? It was unset unless %o was used in XferCommand, but I'm not sure what the use case for an XferCommand without %o would be.
When %o was added, there were cases in the wild where users had a downloader that did not support resuming the download. No sure this would be the case now... Allan
On 06/11/19 at 11:06am, Allan McRae wrote:
On 10/6/19 6:50 am, Andrew Gregory wrote:
Does anybody know what usepart was for? It was unset unless %o was used in XferCommand, but I'm not sure what the use case for an XferCommand without %o would be.
When %o was added, there were cases in the wild where users had a downloader that did not support resuming the download. No sure this would be the case now...
I assume nobody minds if that possibility is removed.
On 10/6/19 6:50 am, Andrew Gregory wrote:
---
A few changes I omitted from the initial patch.
Does anybody know what usepart was for? It was unset unless %o was used in XferCommand, but I'm not sure what the use case for an XferCommand without %o would be.
According to the man page, %o is optional. I'm OK for that to change... Allan
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- v2: removes _alpm_prefix and moves to util-common lib/libalpm/hook.c | 119 +-------------------------------------- src/common/util-common.c | 112 ++++++++++++++++++++++++++++++++++++ src/common/util-common.h | 3 + 3 files changed, 118 insertions(+), 116 deletions(-) diff --git a/lib/libalpm/hook.c b/lib/libalpm/hook.c index 6143ea0f..9d1aa215 100644 --- a/lib/libalpm/hook.c +++ b/lib/libalpm/hook.c @@ -17,7 +17,6 @@ * along with this program. If not, see <http://www.gnu.org/licenses/>. */ -#include <ctype.h> #include <dirent.h> #include <errno.h> #include <limits.h> @@ -71,23 +70,12 @@ static void _alpm_trigger_free(struct _alpm_trigger_t *trigger) } } -static void _alpm_wordsplit_free(char **ws) -{ - if(ws) { - char **c; - for(c = ws; *c; c++) { - free(*c); - } - free(ws); - } -} - static void _alpm_hook_free(struct _alpm_hook_t *hook) { if(hook) { free(hook->name); free(hook->desc); - _alpm_wordsplit_free(hook->cmd); + wordsplit_free(hook->cmd); alpm_list_free_inner(hook->triggers, (alpm_list_fn_free) _alpm_trigger_free); alpm_list_free(hook->triggers); alpm_list_free(hook->matches); @@ -158,107 +146,6 @@ static int _alpm_hook_validate(alpm_handle_t *handle, return ret; } -static char **_alpm_wordsplit(char *str) -{ - char *c = str, *end; - char **out = NULL, **outsave; - size_t count = 0; - - if(str == NULL) { - errno = EINVAL; - return NULL; - } - - for(c = str; isspace(*c); c++); - while(*c) { - size_t wordlen = 0; - - /* extend our array */ - outsave = out; - if((out = realloc(out, (count + 1) * sizeof(char*))) == NULL) { - out = outsave; - goto error; - } - - /* calculate word length and check for unbalanced quotes */ - for(end = c; *end && !isspace(*end); end++) { - if(*end == '\'' || *end == '"') { - char quote = *end; - while(*(++end) && *end != quote) { - if(*end == '\\' && *(end + 1) == quote) { - end++; - } - wordlen++; - } - if(*end != quote) { - errno = EINVAL; - goto error; - } - } else { - if(*end == '\\' && (end[1] == '\'' || end[1] == '"')) { - end++; /* skip the '\\' */ - } - wordlen++; - } - } - - if(wordlen == (size_t) (end - c)) { - /* no internal quotes or escapes, copy it the easy way */ - if((out[count++] = strndup(c, wordlen)) == NULL) { - goto error; - } - } else { - /* manually copy to remove quotes and escapes */ - char *dest = out[count++] = malloc(wordlen + 1); - if(dest == NULL) { goto error; } - while(c < end) { - if(*c == '\'' || *c == '"') { - char quote = *c; - /* we know there must be a matching end quote, - * no need to check for '\0' */ - for(c++; *c != quote; c++) { - if(*c == '\\' && *(c + 1) == quote) { - c++; - } - *(dest++) = *c; - } - c++; - } else { - if(*c == '\\' && (c[1] == '\'' || c[1] == '"')) { - c++; /* skip the '\\' */ - } - *(dest++) = *(c++); - } - } - *dest = '\0'; - } - - if(*end == '\0') { - break; - } else { - for(c = end + 1; isspace(*c); c++); - } - } - - outsave = out; - if((out = realloc(out, (count + 1) * sizeof(char*))) == NULL) { - out = outsave; - goto error; - } - - out[count++] = NULL; - - return out; - -error: - /* can't use wordsplit_free here because NULL has not been appended */ - while(count) { - free(out[--count]); - } - free(out); - return NULL; -} - static int _alpm_hook_parse_cb(const char *file, int line, const char *section, char *key, char *value, void *data) { @@ -347,9 +234,9 @@ static int _alpm_hook_parse_cb(const char *file, int line, } else if(strcmp(key, "Exec") == 0) { if(hook->cmd != NULL) { warning(_("hook %s line %d: overwriting previous definition of %s\n"), file, line, "Exec"); - _alpm_wordsplit_free(hook->cmd); + wordsplit_free(hook->cmd); } - if((hook->cmd = _alpm_wordsplit(value)) == NULL) { + if((hook->cmd = wordsplit(value)) == NULL) { if(errno == EINVAL) { error(_("hook %s line %d: invalid value %s\n"), file, line, value); } else { diff --git a/src/common/util-common.c b/src/common/util-common.c index 3aa0eac9..fd91ecec 100644 --- a/src/common/util-common.c +++ b/src/common/util-common.c @@ -197,3 +197,115 @@ char *strndup(const char *s, size_t n) return (char *)memcpy(new, s, len); } #endif + +void wordsplit_free(char **ws) +{ + if(ws) { + char **c; + for(c = ws; *c; c++) { + free(*c); + } + free(ws); + } +} + +char **wordsplit(const char *str) +{ + const char *c = str, *end; + char **out = NULL, **outsave; + size_t count = 0; + + if(str == NULL) { + errno = EINVAL; + return NULL; + } + + for(c = str; isspace(*c); c++); + while(*c) { + size_t wordlen = 0; + + /* extend our array */ + outsave = out; + if((out = realloc(out, (count + 1) * sizeof(char*))) == NULL) { + out = outsave; + goto error; + } + + /* calculate word length and check for unbalanced quotes */ + for(end = c; *end && !isspace(*end); end++) { + if(*end == '\'' || *end == '"') { + char quote = *end; + while(*(++end) && *end != quote) { + if(*end == '\\' && *(end + 1) == quote) { + end++; + } + wordlen++; + } + if(*end != quote) { + errno = EINVAL; + goto error; + } + } else { + if(*end == '\\' && (end[1] == '\'' || end[1] == '"')) { + end++; /* skip the '\\' */ + } + wordlen++; + } + } + + if(wordlen == (size_t) (end - c)) { + /* no internal quotes or escapes, copy it the easy way */ + if((out[count++] = strndup(c, wordlen)) == NULL) { + goto error; + } + } else { + /* manually copy to remove quotes and escapes */ + char *dest = out[count++] = malloc(wordlen + 1); + if(dest == NULL) { goto error; } + while(c < end) { + if(*c == '\'' || *c == '"') { + char quote = *c; + /* we know there must be a matching end quote, + * no need to check for '\0' */ + for(c++; *c != quote; c++) { + if(*c == '\\' && *(c + 1) == quote) { + c++; + } + *(dest++) = *c; + } + c++; + } else { + if(*c == '\\' && (c[1] == '\'' || c[1] == '"')) { + c++; /* skip the '\\' */ + } + *(dest++) = *(c++); + } + } + *dest = '\0'; + } + + if(*end == '\0') { + break; + } else { + for(c = end + 1; isspace(*c); c++); + } + } + + outsave = out; + if((out = realloc(out, (count + 1) * sizeof(char*))) == NULL) { + out = outsave; + goto error; + } + + out[count++] = NULL; + + return out; + +error: + /* can't use wordsplit_free here because NULL has not been appended */ + while(count) { + free(out[--count]); + } + free(out); + return NULL; +} diff --git a/src/common/util-common.h b/src/common/util-common.h index 3434e1eb..7ec588cc 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -30,6 +30,9 @@ int llstat(char *path, struct stat *buf); char *safe_fgets(char *s, int size, FILE *stream); +void wordsplit_free(char **ws); +char **wordsplit(const char *str); + size_t strtrim(char *str); #ifndef HAVE_STRNDUP -- 2.23.0
Converts an argc/argv pair to a string for presentation to the user. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/pacman.c | 26 +++++--------------------- src/pacman/util.c | 23 +++++++++++++++++++++++ src/pacman/util.h | 1 + 3 files changed, 29 insertions(+), 21 deletions(-) diff --git a/src/pacman/pacman.c b/src/pacman/pacman.c index b7406cea..c8e86b9d 100644 --- a/src/pacman/pacman.c +++ b/src/pacman/pacman.c @@ -1066,28 +1066,12 @@ static int parseargs(int argc, char *argv[]) */ static void cl_to_log(int argc, char *argv[]) { - size_t size = 0; - int i; - for(i = 0; i < argc; i++) { - size += strlen(argv[i]) + 1; + char *cl_text = arg_to_string(argc, argv); + if(cl_text) { + alpm_logaction(config->handle, PACMAN_CALLER_PREFIX, + "Running '%s'\n", cl_text); + free(cl_text); } - if(!size) { - return; - } - char *cl_text = malloc(size); - if(!cl_text) { - return; - } - char *p = cl_text; - for(i = 0; i + 1 < argc; i++) { - strcpy(p, argv[i]); - p += strlen(argv[i]); - *p++ = ' '; - } - strcpy(p, argv[i]); - alpm_logaction(config->handle, PACMAN_CALLER_PREFIX, - "Running '%s'\n", cl_text); - free(cl_text); } /** Main function. diff --git a/src/pacman/util.c b/src/pacman/util.c index 8f6290db..68cdb2e9 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -1771,3 +1771,26 @@ int pm_vfprintf(FILE *stream, alpm_loglevel_t level, const char *format, va_list ret = vfprintf(stream, format, args); return ret; } + +char *arg_to_string(int argc, char *argv[]) +{ + char *cl_text, *p; + size_t size = 0; + int i; + for(i = 0; i < argc; i++) { + size += strlen(argv[i]) + 1; + } + if(!size) { + return NULL; + } + if(!(cl_text = malloc(size))) { + return NULL; + } + for(p = cl_text, i = 0; i + 1 < argc; i++) { + strcpy(p, argv[i]); + p += strlen(argv[i]); + *p++ = ' '; + } + strcpy(p, argv[i]); + return cl_text; +} diff --git a/src/pacman/util.h b/src/pacman/util.h index a1fbef46..4b049849 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -76,6 +76,7 @@ int multiselect_question(char *array, int count); int colon_printf(const char *format, ...) __attribute__((format(printf, 1, 2))); int yesno(const char *format, ...) __attribute__((format(printf, 1, 2))); int noyes(const char *format, ...) __attribute__((format(printf, 1, 2))); +char *arg_to_string(int argc, char *argv[]); int pm_printf(alpm_loglevel_t level, const char *format, ...) __attribute__((format(printf,2,3))); int pm_asprintf(char **string, const char *format, ...) __attribute__((format(printf,2,3))); -- 2.23.0
Hi Andrew,
- for(i = 0; i + 1 < argc; i++) { - strcpy(p, argv[i]); - p += strlen(argv[i]); - *p++ = ' '; - }
stpcpy(3)? -- Cheers, Ralph.
On 12/10/19 8:20 pm, Ralph Corderoy wrote:
Hi Andrew,
- for(i = 0; i + 1 < argc; i++) { - strcpy(p, argv[i]); - p += strlen(argv[i]); - *p++ = ' '; - }
stpcpy(3)?
This patch is just relocating that code block. A change to stpcpy would be a separate patch. Allan
system() runs the provided command via a shell, which is subject to command injection. Even though pacman already provides a mechanism to sign and verify the databases containing the urls, certain distributions have yet to get their act together and start signing databases, leaving them vulnerable to MITM attacks. Replacing the system call with an almost equivalent exec call removes the possibility of a shell-injection attack for those users. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- v2: * properly deal with signals * pass errno via pipe instead of mmap * fix debug logging src/pacman/conf.c | 129 ++++++++++++++++++++++++---- src/pacman/conf.h | 2 + test/pacman/tests/sync200.py | 2 +- test/pacman/tests/xfercommand001.py | 2 +- 4 files changed, 116 insertions(+), 19 deletions(-) diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 2d8518c4..9a39bba9 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -29,6 +29,7 @@ #include <sys/stat.h> #include <sys/types.h> #include <sys/utsname.h> /* uname */ +#include <sys/wait.h> #include <unistd.h> /* pacman */ @@ -153,6 +154,7 @@ int config_free(config_t *oldconfig) free(oldconfig->print_format); free(oldconfig->arch); free(oldconfig); + wordsplit_free(oldconfig->xfercommand_argv); return 0; } @@ -201,18 +203,86 @@ static char *get_tempfile(const char *path, const char *filename) return tempfile; } +/* system()/exec() hybrid function allowing exec()-style direct execution + * of a command with the simplicity of system() + * - not thread-safe + * - errno may be set by fork(), pipe(), or execvp() + */ +static int systemvp(const char *file, char *const argv[]) +{ + int pid, err = 0, ret = -1, err_fd[2]; + sigset_t oldblock; + struct sigaction sa_ign = { .sa_handler = SIG_IGN }, oldint, oldquit; + + if(pipe(err_fd) != 0) { + return -1; + } + + sigaction(SIGINT, &sa_ign, &oldint); + sigaction(SIGQUIT, &sa_ign, &oldquit); + sigaddset(&sa_ign.sa_mask, SIGCHLD); + sigprocmask(SIG_BLOCK, &sa_ign.sa_mask, &oldblock); + + pid = fork(); + + /* child */ + if(pid == 0) { + close(err_fd[0]); + fcntl(err_fd[1], F_SETFD, FD_CLOEXEC); + + /* restore signal handling for the child to inherit */ + sigaction(SIGINT, &oldint, NULL); + sigaction(SIGQUIT, &oldquit, NULL); + sigprocmask(SIG_SETMASK, &oldblock, NULL); + + execvp(file, argv); + + /* execvp failed, pass the error back to the parent */ + while(write(err_fd[1], &errno, sizeof(errno)) == -1 && errno == EINTR); + _Exit(127); + } + + /* parent */ + close(err_fd[1]); + + if(pid != -1) { + int wret; + while((wret = waitpid(pid, &ret, 0)) == -1 && errno == EINTR); + if(wret > 0) { + while(read(err_fd[0], &err, sizeof(err)) == -1 && errno == EINTR); + } + } else { + /* fork failed, make sure errno is preserved after cleanup */ + err = errno; + } + + close(err_fd[0]); + + sigaction(SIGINT, &oldint, NULL); + sigaction(SIGQUIT, &oldquit, NULL); + sigprocmask(SIG_SETMASK, &oldblock, NULL); + + if(err) { + errno = err; + ret = -1; + } + + return ret; +} + /** External fetch callback */ static int download_with_xfercommand(const char *url, const char *localpath, int force) { int ret = 0, retval; int usepart = 0; - int cwdfd; + int cwdfd = -1; struct stat st; - char *parsedcmd, *tempcmd; char *destfile, *tempfile, *filename; + const char **argv; + size_t i; - if(!config->xfercommand) { + if(!config->xfercommand_argv) { return -1; } @@ -230,17 +300,26 @@ static int download_with_xfercommand(const char *url, const char *localpath, unlink(destfile); } - tempcmd = strdup(config->xfercommand); - /* replace all occurrences of %o with fn.part */ - if(strstr(tempcmd, "%o")) { - usepart = 1; - parsedcmd = strreplace(tempcmd, "%o", tempfile); - free(tempcmd); - tempcmd = parsedcmd; + if((argv = calloc(config->xfercommand_argc + 1, sizeof(char*))) == NULL) { + size_t bytes = (config->xfercommand_argc + 1) * sizeof(char*); + pm_printf(ALPM_LOG_ERROR, + _n("malloc failure: could not allocate %zu byte\n", + "malloc failure: could not allocate %zu bytes\n", + bytes), + bytes); + goto cleanup; + } + + for(i = 0; i <= config->xfercommand_argc; i++) { + const char *val = config->xfercommand_argv[i]; + if(val && strcmp(val, "%o") == 0) { + usepart = 1; + val = tempfile; + } else if(val && strcmp(val, "%u") == 0) { + val = url; + } + argv[i] = val; } - /* replace all occurrences of %u with the download URL */ - parsedcmd = strreplace(tempcmd, "%u", url); - free(tempcmd); /* save the cwd so we can restore it later */ do { @@ -256,9 +335,15 @@ static int download_with_xfercommand(const char *url, const char *localpath, ret = -1; goto cleanup; } - /* execute the parsed command via /bin/sh -c */ - pm_printf(ALPM_LOG_DEBUG, "running command: %s\n", parsedcmd); - retval = system(parsedcmd); + + if(config->logmask & ALPM_LOG_DEBUG) { + char *cmd = arg_to_string(config->xfercommand_argc, (char**)argv); + if(cmd) { + pm_printf(ALPM_LOG_DEBUG, "running command: %s\n", cmd); + free(cmd); + } + } + retval = systemvp(argv[0], (char**)argv); if(retval == -1) { pm_printf(ALPM_LOG_WARNING, _("running XferCommand: fork failed!\n")); @@ -296,7 +381,6 @@ cleanup: } free(destfile); free(tempfile); - free(parsedcmd); return ret; } @@ -544,6 +628,17 @@ static int _parse_options(const char *key, char *value, pm_printf(ALPM_LOG_DEBUG, "config: logfile: %s\n", value); } } else if(strcmp(key, "XferCommand") == 0) { + char **c; + if((config->xfercommand_argv = wordsplit(value)) == NULL) { + pm_printf(ALPM_LOG_WARNING, + _("config file %s, line %d: invalid value for '%s' : '%s'\n"), + file, linenum, "XferCommand", value); + return 1; + } + config->xfercommand_argc = 0; + for(c = config->xfercommand_argv; *c; c++) { + config->xfercommand_argc++; + } config->xfercommand = strdup(value); pm_printf(ALPM_LOG_DEBUG, "config: xfercommand: %s\n", value); } else if(strcmp(key, "CleanMethod") == 0) { diff --git a/src/pacman/conf.h b/src/pacman/conf.h index f45ed436..47df979d 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -125,6 +125,8 @@ typedef struct __config_t { alpm_list_t *noextract; alpm_list_t *overwrite_files; char *xfercommand; + char **xfercommand_argv; + size_t xfercommand_argc; /* our connection to libalpm */ alpm_handle_t *handle; diff --git a/test/pacman/tests/sync200.py b/test/pacman/tests/sync200.py index 2bcdd5d3..18f38b81 100644 --- a/test/pacman/tests/sync200.py +++ b/test/pacman/tests/sync200.py @@ -1,6 +1,6 @@ self.description = "Synchronize the local database" -self.option['XferCommand'] = ['/usr/bin/curl %u > %o'] +self.option['XferCommand'] = ['/usr/bin/curl %u -o %o'] sp1 = pmpkg("spkg1", "1.0-1") sp1.depends = ["spkg2"] diff --git a/test/pacman/tests/xfercommand001.py b/test/pacman/tests/xfercommand001.py index 0d244dc6..0ac99080 100644 --- a/test/pacman/tests/xfercommand001.py +++ b/test/pacman/tests/xfercommand001.py @@ -3,7 +3,7 @@ # this setting forces us to download packages self.cachepkgs = False #wget doesn't support file:// urls. curl does -self.option['XferCommand'] = ['/usr/bin/curl %u > %o'] +self.option['XferCommand'] = ['/usr/bin/curl %u -o %o'] numpkgs = 10 pkgnames = [] -- 2.23.0
On 12/10/19 1:45 pm, Andrew Gregory wrote:
system() runs the provided command via a shell, which is subject to command injection. Even though pacman already provides a mechanism to sign and verify the databases containing the urls, certain distributions have yet to get their act together and start signing databases, leaving them vulnerable to MITM attacks. Replacing the system call with an almost equivalent exec call removes the possibility of a shell-injection attack for those users.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- v2: * properly deal with signals * pass errno via pipe instead of mmap * fix debug logging
src/pacman/conf.c | 129 ++++++++++++++++++++++++---- src/pacman/conf.h | 2 + test/pacman/tests/sync200.py | 2 +- test/pacman/tests/xfercommand001.py | 2 +- 4 files changed, 116 insertions(+), 19 deletions(-)
diff --git a/src/pacman/conf.c b/src/pacman/conf.c index 2d8518c4..9a39bba9 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -29,6 +29,7 @@ #include <sys/stat.h> #include <sys/types.h> #include <sys/utsname.h> /* uname */ +#include <sys/wait.h> #include <unistd.h>
/* pacman */ @@ -153,6 +154,7 @@ int config_free(config_t *oldconfig) free(oldconfig->print_format); free(oldconfig->arch); free(oldconfig); + wordsplit_free(oldconfig->xfercommand_argv);
This line needs to be one higher. A
On 12/10/19 1:45 pm, Andrew Gregory wrote:
system() runs the provided command via a shell, which is subject to command injection. Even though pacman already provides a mechanism to sign and verify the databases containing the urls, certain distributions have yet to get their act together and start signing databases, leaving them vulnerable to MITM attacks. Replacing the system call with an almost equivalent exec call removes the possibility of a shell-injection attack for those users.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com>
<snip>
@@ -230,17 +300,26 @@ static int download_with_xfercommand(const char *url, const char *localpath, unlink(destfile); }
- tempcmd = strdup(config->xfercommand); - /* replace all occurrences of %o with fn.part */ - if(strstr(tempcmd, "%o")) { - usepart = 1; - parsedcmd = strreplace(tempcmd, "%o", tempfile); - free(tempcmd); - tempcmd = parsedcmd; + if((argv = calloc(config->xfercommand_argc + 1, sizeof(char*))) == NULL) {
need to free this at the end.
On 10/12/19 at 09:11pm, Allan McRae wrote:
On 12/10/19 1:45 pm, Andrew Gregory wrote:
system() runs the provided command via a shell, which is subject to command injection. Even though pacman already provides a mechanism to sign and verify the databases containing the urls, certain distributions have yet to get their act together and start signing databases, leaving them vulnerable to MITM attacks. Replacing the system call with an almost equivalent exec call removes the possibility of a shell-injection attack for those users.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com>
<snip>
@@ -230,17 +300,26 @@ static int download_with_xfercommand(const char *url, const char *localpath, unlink(destfile); }
- tempcmd = strdup(config->xfercommand); - /* replace all occurrences of %o with fn.part */ - if(strstr(tempcmd, "%o")) { - usepart = 1; - parsedcmd = strreplace(tempcmd, "%o", tempfile); - free(tempcmd); - tempcmd = parsedcmd; + if((argv = calloc(config->xfercommand_argc + 1, sizeof(char*))) == NULL) {
need to free this at the end.
Updated patch pushed to my repo that fixes this and the misplaced free and also corrects the indenting in systemvp to use tabs.
On Sun, Jun 09, 2019 at 10:13:55AM -0700, Andrew Gregory wrote:
---
systemvp should pretty much be a drop-in replacement for system with the exception that it takes an argv array and uses exec. If anybody wants to play with it to stress test it a little, I have a self-contained copy and test program at: https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c
TODO: * update docs * fix debug logging * should the command be run with PATH lookup (execv vs execvp)? * Is the use of mmap with MAP_ANONYMOUS okay? MAP_ANONYMOUS is not POSIX but "most systems also support MAP_ANONYMOUS (or its synonym MAP_ANON)" (mmap(2)). * should we reset signals prior to exec'ing like we do with hooks/scripts?
This issue was assigned CVE-2019-18182. https://security.archlinux.org/CVE-2019-18182 I'm fixing the AVG whenever pacman 5.2 is released if Xfer isn't included. -- Morten Linderud PGP: 9C02FF419FECBE16
On Thu, Oct 17, 2019 at 05:01:46PM +0200, Morten Linderud wrote:
On Sun, Jun 09, 2019 at 10:13:55AM -0700, Andrew Gregory wrote:
---
systemvp should pretty much be a drop-in replacement for system with the exception that it takes an argv array and uses exec. If anybody wants to play with it to stress test it a little, I have a self-contained copy and test program at: https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c
TODO: * update docs * fix debug logging * should the command be run with PATH lookup (execv vs execvp)? * Is the use of mmap with MAP_ANONYMOUS okay? MAP_ANONYMOUS is not POSIX but "most systems also support MAP_ANONYMOUS (or its synonym MAP_ANON)" (mmap(2)). * should we reset signals prior to exec'ing like we do with hooks/scripts?
This issue was assigned CVE-2019-18182.
https://security.archlinux.org/CVE-2019-18182
I'm fixing the AVG whenever pacman 5.2 is released if Xfer isn't included.
Uh. I might not have paid attention. Eli mentioned on -security Xfer might not be included in the upcomming release, but then anthraxx pointed out it's in master :o Whats the status? -- Morten Linderud PGP: 9C02FF419FECBE16
On 10/17/19 11:04 AM, Morten Linderud wrote:
On Thu, Oct 17, 2019 at 05:01:46PM +0200, Morten Linderud wrote:
On Sun, Jun 09, 2019 at 10:13:55AM -0700, Andrew Gregory wrote:
---
systemvp should pretty much be a drop-in replacement for system with the exception that it takes an argv array and uses exec. If anybody wants to play with it to stress test it a little, I have a self-contained copy and test program at: https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c
TODO: * update docs * fix debug logging * should the command be run with PATH lookup (execv vs execvp)? * Is the use of mmap with MAP_ANONYMOUS okay? MAP_ANONYMOUS is not POSIX but "most systems also support MAP_ANONYMOUS (or its synonym MAP_ANON)" (mmap(2)). * should we reset signals prior to exec'ing like we do with hooks/scripts?
This issue was assigned CVE-2019-18182.
https://security.archlinux.org/CVE-2019-18182
I'm fixing the AVG whenever pacman 5.2 is released if Xfer isn't included.
Uh. I might not have paid attention. Eli mentioned on -security Xfer might not be included in the upcomming release, but then anthraxx pointed out it's in master :o Whats the status?
Just to clarify, "might not be included in the upcoming release" was before the v2 patch series posted on Friday. Before then, it was unclear if the v1 patch series (which was marked as WIP with some TODO items) would be finished before the upcoming release. This has landed in master as the following commit: https://git.archlinux.org/pacman.git/commit/?id=808a4f15ce82d2ed7eeb06de73d0... And is mentioned in the NEWS file which is prepared here: https://patchwork.archlinux.org/patch/1280/ -- Eli Schwartz Bug Wrangler and Trusted User
On Thu, Oct 17, 2019 at 11:47:58AM -0400, Eli Schwartz wrote:
On 10/17/19 11:04 AM, Morten Linderud wrote:
On Thu, Oct 17, 2019 at 05:01:46PM +0200, Morten Linderud wrote:
On Sun, Jun 09, 2019 at 10:13:55AM -0700, Andrew Gregory wrote:
---
systemvp should pretty much be a drop-in replacement for system with the exception that it takes an argv array and uses exec. If anybody wants to play with it to stress test it a little, I have a self-contained copy and test program at: https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c
TODO: * update docs * fix debug logging * should the command be run with PATH lookup (execv vs execvp)? * Is the use of mmap with MAP_ANONYMOUS okay? MAP_ANONYMOUS is not POSIX but "most systems also support MAP_ANONYMOUS (or its synonym MAP_ANON)" (mmap(2)). * should we reset signals prior to exec'ing like we do with hooks/scripts?
This issue was assigned CVE-2019-18182.
https://security.archlinux.org/CVE-2019-18182
I'm fixing the AVG whenever pacman 5.2 is released if Xfer isn't included.
Uh. I might not have paid attention. Eli mentioned on -security Xfer might not be included in the upcomming release, but then anthraxx pointed out it's in master :o Whats the status?
Just to clarify, "might not be included in the upcoming release" was before the v2 patch series posted on Friday. Before then, it was unclear if the v1 patch series (which was marked as WIP with some TODO items) would be finished before the upcoming release.
This has landed in master as the following commit:
https://git.archlinux.org/pacman.git/commit/?id=808a4f15ce82d2ed7eeb06de73d0...
And is mentioned in the NEWS file which is prepared here: https://patchwork.archlinux.org/patch/1280/
Ack thanks. That was what anthraxx also wrote to me but the previous mail was sent a bit too quickly. -- Morten Linderud PGP: 9C02FF419FECBE16
On 10/6/19 3:13 am, Andrew Gregory wrote:
Not technically related to INI parsing, but we use it with INI files.
It seems a strange place to put these functions... And they should not have an _alpm prefix if being used outside libalpm. How about putting them util-common without the prefix? Allan
participants (5)
-
Allan McRae
-
Andrew Gregory
-
Eli Schwartz
-
Morten Linderud
-
Ralph Corderoy