Re: [pacman-dev] [PATCH 1/2] Remove most usages of strncmp()

On Wed, Jul 6, 2011 at 3:20 AM, 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 ---  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);