[Qemu-devel] [PATCH] qemu-io: initialize progname with error_set_progname()

[Qemu-devel] [PATCH] qemu-io: initialize progname with error_set_progname()

[Hitoshi Mitake]

Calling error_get_progname() in the context of qemu-io can cause
segmentation fault because qemu-io doesn't initialize its progname
with error_set_progname(). This patch adds the initialization.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
---
 qemu-io.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qemu-io.c b/qemu-io.c
index b55a550..6f08a91 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -395,6 +395,7 @@ int main(int argc, char **argv)
 #endif
 
     progname = basename(argv[0]);
+    error_set_progname(progname);
     qemu_init_exec_dir(argv[0]);
 
     while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
-- 
1.8.3.2

Re: [Qemu-devel] [PATCH] qemu-io: initialize progname with error_set_progname()

[Markus Armbruster]

Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> writes:

> Calling error_get_progname() in the context of qemu-io can cause
> segmentation fault because qemu-io doesn't initialize its progname
> with error_set_progname(). This patch adds the initialization.
>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> ---
>  qemu-io.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/qemu-io.c b/qemu-io.c
> index b55a550..6f08a91 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -395,6 +395,7 @@ int main(int argc, char **argv)
>  #endif
>  
>      progname = basename(argv[0]);
> +    error_set_progname(progname);
>      qemu_init_exec_dir(argv[0]);
>  
>      while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {

Global variable progname could be replaced by error_get_progname().

Can pass argv[0] directly, without basename().

Do any other programs besides qemu-io and qemu proper need to call
error_set_progname()?  Quick grep for main() finds

    qemu-bridge-helper.c
    qemu-io.c
    qemu-nbd.c
    bsd-user/main.c
    disas/m68k.c
    fsdev/virtfs-proxy-helper.c
    libcacard/link_test.c
    libcacard/vscclient.c
    linux-user/main.c
    qga/main.c
    target-ppc/mfrom_table_gen.c
    pc-bios/s390-ccw/main.c

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] qemu-io: initialize progname with error_set_progname()

[Hitoshi Mitake]

At Mon, 11 Aug 2014 13:20:38 +0200,
Markus Armbruster wrote:
> 
> Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp> writes:
> 
> > Calling error_get_progname() in the context of qemu-io can cause
> > segmentation fault because qemu-io doesn't initialize its progname
> > with error_set_progname(). This patch adds the initialization.
> >
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > ---
> >  qemu-io.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/qemu-io.c b/qemu-io.c
> > index b55a550..6f08a91 100644
> > --- a/qemu-io.c
> > +++ b/qemu-io.c
> > @@ -395,6 +395,7 @@ int main(int argc, char **argv)
> >  #endif
> >  
> >      progname = basename(argv[0]);
> > +    error_set_progname(progname);
> >      qemu_init_exec_dir(argv[0]);
> >  
> >      while ((c = getopt_long(argc, argv, sopt, lopt, &opt_index)) != -1) {
> 
> Global variable progname could be replaced by error_get_progname().

Hi Markus, thanks for your comments.

Yes, replacing is possible but I think we don't have to do it. The
main purpose of error_set_progname() is improving output of
error_report(). If program uses its binary name frequently, caching it
in other variable is suitable.

> 
> Can pass argv[0] directly, without basename().

Yes, as Stefan pointed, I'll do so in v2.

> 
> Do any other programs besides qemu-io and qemu proper need to call
> error_set_progname()?  Quick grep for main() finds
> 
>     qemu-bridge-helper.c
>     qemu-io.c
>     qemu-nbd.c
>     bsd-user/main.c
>     disas/m68k.c
>     fsdev/virtfs-proxy-helper.c
>     libcacard/link_test.c
>     libcacard/vscclient.c
>     linux-user/main.c
>     qga/main.c
>     target-ppc/mfrom_table_gen.c
>     pc-bios/s390-ccw/main.c
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

I think
    qemu-io.c
    qemu-nbd.c
    bsd-user/main.c
    linux-user/main.c
    qga/main.c
should call error_get_progname() because they seem to call
error_get_progname() potentially. But others seem to be simple
utilities so calling the func might be needless.

I'll update the above utilities in v2 (except bsd-user/main.c, because
I don't have BSD environment and cannot check the change:( ).

BTW, should I split patches for each program? They seem to be
maintained by different maintainers. But the change is really trivial
so packing them in one patch is also reasonable. I'd like to hear your
opinion.

Thanks,
Hitoshi

Re: [Qemu-devel] [PATCH] qemu-io: initialize progname with error_set_progname()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

On Mon, Aug 11, 2014 at 03:50:03PM +0900, Hitoshi Mitake wrote:
> Calling error_get_progname() in the context of qemu-io can cause
> segmentation fault because qemu-io doesn't initialize its progname
> with error_set_progname(). This patch adds the initialization.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> ---
>  qemu-io.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/qemu-io.c b/qemu-io.c
> index b55a550..6f08a91 100644
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -395,6 +395,7 @@ int main(int argc, char **argv)
>  #endif
>  
>      progname = basename(argv[0]);
> +    error_set_progname(progname);

From the basename(3) man page:

  These  functions  may return pointers to statically allocated memory which may be overwritten by subsequent calls.

Since error_set_progname() does not copy its argument, it is not safe to
pass the basename(3) return value.  Just pass in argv[0] like vl.c does.

Please also update the commit description to make it clear that
qemu-io currently does not use error_get_progname() so the crash doesn't
occur in practice (I guess you discovered it while modifying the code).

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] qemu-io: initialize progname with error_set_progname()

[Hitoshi Mitake]

At Mon, 11 Aug 2014 15:38:21 +0100,
Stefan Hajnoczi wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Mon, Aug 11, 2014 at 03:50:03PM +0900, Hitoshi Mitake wrote:
> > Calling error_get_progname() in the context of qemu-io can cause
> > segmentation fault because qemu-io doesn't initialize its progname
> > with error_set_progname(). This patch adds the initialization.
> > 
> > Cc: Kevin Wolf <kwolf@redhat.com>
> > Cc: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Hitoshi Mitake <mitake.hitoshi@lab.ntt.co.jp>
> > ---
> >  qemu-io.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/qemu-io.c b/qemu-io.c
> > index b55a550..6f08a91 100644
> > --- a/qemu-io.c
> > +++ b/qemu-io.c
> > @@ -395,6 +395,7 @@ int main(int argc, char **argv)
> >  #endif
> >  
> >      progname = basename(argv[0]);
> > +    error_set_progname(progname);

Hi Stefan, thanks for your comments.

> 
> From the basename(3) man page:
> 
>   These  functions  may return pointers to statically allocated memory which may be overwritten by subsequent calls.
> 
> Since error_set_progname() does not copy its argument, it is not safe to
> pass the basename(3) return value.  Just pass in argv[0] like vl.c does.

Thanks for your pointing. I'll update it in v2.

> 
> Please also update the commit description to make it clear that
> qemu-io currently does not use error_get_progname() so the crash doesn't
> occur in practice (I guess you discovered it while modifying the code).
> [2  <application/pgp-signature (7bit)>]
> 

Yes, as you guess, I discoverd it during modifying sheepdog block
driver for internal purpose. The patch is just for proactive action.

Thanks,
Hitoshi