[pacman-dev] [PATCH 1/2] Remove most usages of strncmp()
The supposed safety blanket of this function is better handled by explicit length checking and usages of strlen() on known NULL-terminated strings rather than hoping things fit in a buffer. We also have no need to fully fill a PATH_MAX length variable with NULLs every time as long as a single terminating byte is there. Remove usages of it by using strcpy() or memcpy() as appropriate, after doing length checks via strlen(). Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/handle.c | 2 +- src/pacman/query.c | 17 ++++++++++------- src/pacman/util.c | 11 ++++------- src/util/vercmp.c | 2 +- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 22f3fc8..b48d5b6 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -298,7 +298,7 @@ static char *canonicalize_path(const char *path) { len += 1; } CALLOC(new_path, len + 1, sizeof(char), return NULL); - strncpy(new_path, path, len); + strcpy(new_path, path); new_path[len - 1] = '/'; return new_path; } diff --git a/src/pacman/query.c b/src/pacman/query.c index 045dc7f..4fc4584 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -110,8 +110,7 @@ static int query_fileowner(alpm_list_t *targets) int ret = 0; char path[PATH_MAX]; const char *root; - char *append; - size_t max_length; + size_t rootlen; alpm_list_t *t; alpm_db_t *db_local; @@ -125,9 +124,13 @@ static int query_fileowner(alpm_list_t *targets) * once, then we can just overwrite whatever file was there on the previous * iteration. */ root = alpm_option_get_root(config->handle); - strncpy(path, root, PATH_MAX - 1); - append = path + strlen(path); - max_length = PATH_MAX - (append - path) - 1; + rootlen = strlen(root); + if(rootlen + 1 > PATH_MAX) { + /* we are in trouble here */ + pm_fprintf(stderr, ALPM_LOG_ERROR, _("path too long: %s%s\n"), root, ""); + return 1; + } + strcpy(path, root); db_local = alpm_option_get_localdb(config->handle); @@ -208,11 +211,11 @@ static int query_fileowner(alpm_list_t *targets) continue; } - if(strlen(pkgfile) > max_length) { + if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) { pm_fprintf(stderr, ALPM_LOG_ERROR, _("path too long: %s%s\n"), root, pkgfile); } /* concatenate our file and the root path */ - strcpy(append, pkgfile); + strcpy(path + rootlen, pkgfile); pdname = mdirname(path); ppath = resolve_path(pdname); diff --git a/src/pacman/util.c b/src/pacman/util.c index 9ced7aa..203f927 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -361,22 +361,21 @@ char *strreplace(const char *str, const char *needle, const char *replace) * x "size difference between replace and needle" */ newsz = strlen(str) + 1 + alpm_list_count(list) * (replacesz - needlesz); - newstr = malloc(newsz); + newstr = calloc(newsz, sizeof(char)); if(!newstr) { return NULL; } - *newstr = '\0'; p = str; newp = newstr; for(i = list; i; i = alpm_list_next(i)) { q = alpm_list_getdata(i); - if(q > p){ + if(q > p) { /* add chars between this occurence and last occurence, if any */ - strncpy(newp, p, (size_t)(q - p)); + memcpy(newp, p, (size_t)(q - p)); newp += q - p; } - strncpy(newp, replace, replacesz); + memcpy(newp, replace, replacesz); newp += replacesz; p = q + needlesz; } @@ -385,9 +384,7 @@ char *strreplace(const char *str, const char *needle, const char *replace) if(*p) { /* add the rest of 'p' */ strcpy(newp, p); - newp += strlen(p); } - *newp = '\0'; return newstr; } diff --git a/src/util/vercmp.c b/src/util/vercmp.c index 88cf49a..f4356fb 100644 --- a/src/util/vercmp.c +++ b/src/util/vercmp.c @@ -20,7 +20,7 @@ #include <stdlib.h> #include <stdio.h> /* printf */ -#include <string.h> /* strncpy */ +#include <string.h> #define BASENAME "vercmp" -- 1.7.6
We did some funny stuff here before to allow specifying fully-qualified package names, such as 'testing/gcc' or 'core/gcc'. However, it was done by duplicating code, not to mention an early escape if a repository could not be found for an early target. Something like `pacman -Si foo/bar core/gcc' would not give expected results, although `pacman -Si bar gcc' would. Clean up the code, remove strncpy() usage, and clarify the error messages a bit. Signed-off-by: Dan McGee <dan@archlinux.org> --- src/pacman/sync.c | 78 +++++++++++++++++++--------------------------------- 1 files changed, 29 insertions(+), 49 deletions(-) diff --git a/src/pacman/sync.c b/src/pacman/sync.c index ad6d5e5..dab4f2b 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -443,32 +443,27 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) if(targets) { for(i = targets; i; i = alpm_list_next(i)) { - int foundpkg = 0; + const char *target = alpm_list_getdata(i); + char *name = strdup(target); + char *repo, *pkgstr; + int foundpkg = 0, founddb = 0; - char target[512]; /* TODO is this enough space? */ - char *repo = NULL, *pkgstr = NULL; - - strncpy(target, i->data, 512); - pkgstr = strchr(target, '/'); + pkgstr = strchr(name, '/'); if(pkgstr) { - alpm_db_t *db = NULL; - repo = target; + repo = name; *pkgstr = '\0'; ++pkgstr; + } else { + repo = NULL; + pkgstr = name; + } - for(j = syncs; j; j = alpm_list_next(j)) { - db = alpm_list_getdata(j); - if(strcmp(repo, alpm_db_get_name(db)) == 0) { - break; - } - db = NULL; - } - - if(!db) { - pm_fprintf(stderr, ALPM_LOG_ERROR, - _("repository '%s' does not exist\n"), repo); - return 1; + for(j = syncs; j; j = alpm_list_next(j)) { + alpm_db_t *db = alpm_list_getdata(j); + if(repo && strcmp(repo, alpm_db_get_name(db)) != 0) { + continue; } + founddb = 1; for(k = alpm_db_get_pkgcache(db); k; k = alpm_list_next(k)) { alpm_pkg_t *pkg = alpm_list_getdata(k); @@ -479,34 +474,19 @@ static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) break; } } + } - if(!foundpkg) { - pm_fprintf(stderr, ALPM_LOG_ERROR, - _("package '%s' was not found in repository '%s'\n"), pkgstr, repo); - ret++; - } - } else { - pkgstr = target; - - for(j = syncs; j; j = alpm_list_next(j)) { - alpm_db_t *db = alpm_list_getdata(j); - - for(k = alpm_db_get_pkgcache(db); k; k = alpm_list_next(k)) { - alpm_pkg_t *pkg = alpm_list_getdata(k); - - if(strcmp(alpm_pkg_get_name(pkg), pkgstr) == 0) { - dump_pkg_full(pkg, PKG_FROM_SYNCDB, config->op_s_info > 1); - foundpkg = 1; - break; - } - } - } - if(!foundpkg) { - pm_fprintf(stderr, ALPM_LOG_ERROR, - _("package '%s' was not found\n"), pkgstr); - ret++; - } + if(!founddb) { + pm_fprintf(stderr, ALPM_LOG_ERROR, + _("repository '%s' does not exist\n"), repo); + ret++; + } + if(!foundpkg) { + pm_fprintf(stderr, ALPM_LOG_ERROR, + _("package '%s' was not found\n"), target); + ret++; } + free(name); } } else { for(i = syncs; i; i = alpm_list_next(i)) { @@ -630,7 +610,7 @@ static int process_pkg(alpm_pkg_t *pkg) return 0; } -static int process_group(alpm_list_t *dbs, char *group) +static int process_group(alpm_list_t *dbs, const char *group) { int ret = 0; alpm_list_t *i; @@ -676,7 +656,7 @@ cleanup: return ret; } -static int process_targname(alpm_list_t *dblist, char *targname) +static int process_targname(alpm_list_t *dblist, const char *targname) { alpm_pkg_t *pkg = alpm_find_dbs_satisfier(config->handle, dblist, targname); @@ -695,7 +675,7 @@ static int process_targname(alpm_list_t *dblist, char *targname) return process_group(dblist, targname); } -static int process_target(char *target) +static int process_target(const char *target) { /* process targets */ char *targstring = strdup(target); -- 1.7.6
On 06/07/11 05:20, Dan McGee wrote:
The supposed safety blanket of this function is better handled by explicit length checking and usages of strlen() on known NULL-terminated strings rather than hoping things fit in a buffer. We also have no need to fully fill a PATH_MAX length variable with NULLs every time as long as a single terminating byte is there. Remove usages of it by using strcpy() or memcpy() as appropriate, after doing length checks via strlen().
Signed-off-by: Dan McGee<dan@archlinux.org>
Ack-by: Allan
On Wed, Jul 6, 2011 at 3:20 AM, Dan McGee <dan@archlinux.org> wrote:
The supposed safety blanket of this function is better handled by explicit length checking and usages of strlen() on known NULL-terminated strings rather than hoping things fit in a buffer. We also have no need to fully fill a PATH_MAX length variable with NULLs every time as long as a single terminating byte is there. Remove usages of it by using strcpy() or memcpy() as appropriate, after doing length checks via strlen().
Signed-off-by: Dan McGee <dan@archlinux.org> --- lib/libalpm/handle.c | 2 +- src/pacman/query.c | 17 ++++++++++------- src/pacman/util.c | 11 ++++------- src/util/vercmp.c | 2 +- 4 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 22f3fc8..b48d5b6 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -125,9 +124,13 @@ static int query_fileowner(alpm_list_t *targets) * once, then we can just overwrite whatever file was there on the previous * iteration. */ root = alpm_option_get_root(config->handle); - strncpy(path, root, PATH_MAX - 1); - append = path + strlen(path); - max_length = PATH_MAX - (append - path) - 1; + rootlen = strlen(root); + if(rootlen + 1 > PATH_MAX) { + /* we are in trouble here */ + pm_fprintf(stderr, ALPM_LOG_ERROR, _("path too long: %s%s\n"), root, ""); + return 1; + } + strcpy(path, root);
Might be a bit late to the party, but there's potential for a bug to creep in here. If anything is done with "path" before hand, the check becomes incorrect. It should check strlen(path) + rootlen, and is done later on:
@@ -208,11 +211,11 @@ static int query_fileowner(alpm_list_t *targets) continue; }
- if(strlen(pkgfile) > max_length) { + if(rootlen + 1 + strlen(pkgfile) > PATH_MAX) { pm_fprintf(stderr, ALPM_LOG_ERROR, _("path too long: %s%s\n"), root, pkgfile); } /* concatenate our file and the root path */ - strcpy(append, pkgfile); + strcpy(path + rootlen, pkgfile);
pdname = mdirname(path); ppath = resolve_path(pdname);
participants (3)
-
Allan McRae
-
Dan McGee
-
Sebastian Nowicki