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

[Sebastian Nowicki]

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