Re: [PATCH 1/6] move quote_filename into lib/dpkg/path.{c,h} as path_quote_filename

[Guillem Jover <guillem@debian.org>]

Hi!

I've pushed this with few minor modifications. I thought it would be
less work for both of you, and it was just nit picking stuff.
Commmenting anyway for reference.

On Mon, 2009-09-28 at 23:34:28 +0200, Sean Finney wrote:
> From: Sean Finney <seanius@seanius.net>
> 
> this function will be useful for other parts of dpkg, so the function
> has been moved to a more sensible location, the static qualifier removed,
> and its name appropriately prefixed.

I've added the subsystem to the short description, and capitalized the
sentences.

> ---
>  lib/dpkg/path.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/dpkg/path.h |    1 +
>  src/archives.c  |   63 ++----------------------------------------------------
>  3 files changed, 63 insertions(+), 60 deletions(-)
> 
> diff --git a/lib/dpkg/path.c b/lib/dpkg/path.c
> index 8ea891e..ad14a45 100644
> --- a/lib/dpkg/path.c
> +++ b/lib/dpkg/path.c
> @@ -54,3 +55,61 @@ path_skip_slash_dotslash(const char *path)
>  	return path;
>  }
>  
> +/* snprintf(3) doesn't work if format contains %.<nnn>s and an argument has
> + * invalid char for locale, then it returns -1.
> + * ohshite() is ok, but fd_fd_copy(), which is used in tarobject() in this
> + * file, is not ok, because
> + * - fd_fd_copy() == buffer_copy_setup() [include/dpkg.h]
> + * - buffer_copy_setup() uses varbufvprintf(&v, desc, al); [lib/mlib.c]
> + * - varbufvprintf() fails and memory exausted, because it call
> + *    fmt = "backend dpkg-deb during `%.255s'
> + *    arg may contain some invalid char, for example,
> + *    /usr/share/doc/console-tools/examples/unicode/\342\231\252\342\231\254
> + *   in console-tools.
> + *   In this case, if user uses some locale which doesn't support \342\231...,
> + *   vsnprintf() always returns -1 and varbufextend() get called again
> + *   and again until memory is exausted and it aborts.
> + *
> + * So, we need to escape invalid char, probably as in
> + * tar-1.13.19/lib/quotearg.c: quotearg_buffer_restyled()
> + * but here I escape all 8bit chars, in order to be simple.
> + * - ukai@debian.or.jp
> + */

Reformatted slightly the comment.

> diff --git a/src/archives.c b/src/archives.c
> index f9f95a8..d2a6da4 100644
> --- a/src/archives.c
> +++ b/src/archives.c
> @@ -47,6 +47,7 @@
>  #include <dpkg/subproc.h>
>  #include <dpkg/tarfn.h>
>  #include <dpkg/myopt.h>
> +#include <dpkg/path.h>

Placed this one just before <buffer.h>, sorted by complexity.

>  #ifdef WITH_SELINUX
>  #include <selinux/selinux.h>

> @@ -255,7 +198,7 @@ tarfile_skip_one_forward(struct TarInfo *ti,
>  
>      fd_null_copy(tc->backendpipe, ti->Size,
>                   _("skipped unpacking file '%.255s' (replaced or excluded?)"),
> -                 quote_filename(fnamebuf, 256, ti->Name));
> +                 path_quote_filename(fnamebuf, 256, ti->Name));
>      r = ti->Size % TARBLKSZ;
>      if (r > 0)
>        r = safe_read(tc->backendpipe, databuf, TARBLKSZ - r);
> @@ -686,7 +629,7 @@ int tarobject(struct TarInfo *ti) {
>      debug(dbg_eachfiledetail,"tarobject NormalFile[01] open size=%lu",
>            (unsigned long)ti->Size);
>      { char fnamebuf[256];
> -    fd_fd_copy(tc->backendpipe, fd, ti->Size, _("backend dpkg-deb during `%.255s'"),quote_filename(fnamebuf,256,ti->Name));
> +    fd_fd_copy(tc->backendpipe, fd, ti->Size, _("backend dpkg-deb during `%.255s'"),path_quote_filename(fnamebuf,256,ti->Name));

And wrapped this one and added the needed spaces after comma.

>      }
>      r= ti->Size % TARBLKSZ;
>      if (r > 0) r= safe_read(tc->backendpipe,databuf,TARBLKSZ - r);

thanks,
guillem