Message ID | 1407739803-22699-1-git-send-email-mitake.hitoshi@lab.ntt.co.jp |
---|---|
State | New |
Headers | show |
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>
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).
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
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
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) {
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(+)