| Message ID | 1519888086-4207-1-git-send-email-jusual@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Thu, Mar 1, 2018 at 8:08 AM, Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> wrote: > basename(3) and dirname(3) modify their argument and may return > pointers to statically allocated memory which may be overwritten by > subsequent calls. > g_path_get_basename and g_path_get_dirname have no such issues, and > therefore more preferable. > > Signed-off-by: Julia Suvorova <jusual@mail.ru> What about adding a warning for basename()/dirname() usage in scripts/checkpatch.pl ? Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > fsdev/virtfs-proxy-helper.c | 6 +++++- > hw/s390x/s390-ccw.c | 17 +++++++++++------ > hw/vfio/ccw.c | 7 +++++-- > hw/vfio/pci.c | 6 ++++-- > hw/vfio/platform.c | 6 ++++-- > qemu-io.c | 8 +++++++- > qemu-nbd.c | 5 ++++- > qga/commands-posix.c | 4 ++-- > 8 files changed, 42 insertions(+), 17 deletions(-) > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 8e48500..da3452f 100644 > --- a/fsdev/virtfs-proxy-helper.c > +++ b/fsdev/virtfs-proxy-helper.c > @@ -787,6 +787,8 @@ error: > > static void usage(char *prog) > { > + char *base_filename = g_path_get_basename(prog); > + > fprintf(stderr, "usage: %s\n" > " -p|--path <path> 9p path to export\n" > " {-f|--fd <socket-descriptor>} socket file descriptor to be used\n" > @@ -795,7 +797,9 @@ static void usage(char *prog) > " access to this socket\n" > " \tNote: -s & -f can not be used together\n" > " [-n|--nodaemon] Run as a normal program\n", > - basename(prog)); > + base_filename); > + > + g_free(base_filename); > } > > static int process_reply(int sock, int type, > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > index 7fc1c60..460dbab 100644 > --- a/hw/s390x/s390-ccw.c > +++ b/hw/s390x/s390-ccw.c > @@ -34,7 +34,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev, > Error **errp) > { > unsigned int cssid, ssid, devid; > - char dev_path[PATH_MAX] = {0}, *tmp; > + char dev_path[PATH_MAX] = {0}, *dir_name, *dir_path; > > if (!sysfsdev) { > error_setg(errp, "No host device provided"); > @@ -48,18 +48,23 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev, > return; > } > > - cdev->mdevid = g_strdup(basename(dev_path)); > + cdev->mdevid = g_path_get_basename(dev_path); > > - tmp = basename(dirname(dev_path)); > - if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) { > - error_setg_errno(errp, errno, "Failed to read %s", tmp); > - return; > + dir_path = g_path_get_dirname(dev_path); > + dir_name = g_path_get_basename(dir_path); > + if (sscanf(dir_name, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) { > + error_setg_errno(errp, errno, "Failed to read %s", dir_name); > + goto out; > } > > cdev->hostid.cssid = cssid; > cdev->hostid.ssid = ssid; > cdev->hostid.devid = devid; > cdev->hostid.valid = true; > + > +out: > + g_free(dir_path); > + g_free(dir_name); > } > > static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp) > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 16713f2..c0566a9 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -300,7 +300,7 @@ static void vfio_put_device(VFIOCCWDevice *vcdev) > > static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp) > { > - char *tmp, group_path[PATH_MAX]; > + char *tmp, *group_name, group_path[PATH_MAX]; > ssize_t len; > int groupid; > > @@ -317,10 +317,13 @@ static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp) > > group_path[len] = 0; > > - if (sscanf(basename(group_path), "%d", &groupid) != 1) { > + group_name = g_path_get_basename(group_path); > + if (sscanf(group_name, "%d", &groupid) != 1) { > error_setg(errp, "vfio: failed to read %s", group_path); > + g_free(group_name); > return NULL; > } > + g_free(group_name); > > return vfio_get_group(groupid, &address_space_memory, errp); > } > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 033cc8d..ba03136 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2807,7 +2807,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > return; > } > > - vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev)); > + vdev->vbasedev.name = g_path_get_basename(vdev->vbasedev.sysfsdev); > vdev->vbasedev.ops = &vfio_pci_ops; > vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI; > vdev->vbasedev.dev = &vdev->pdev.qdev; > @@ -2824,11 +2824,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > group_path[len] = 0; > > - group_name = basename(group_path); > + group_name = g_path_get_basename(group_path); > if (sscanf(group_name, "%d", &groupid) != 1) { > error_setg_errno(errp, errno, "failed to read %s", group_path); > + g_free(group_name); > goto error; > } > + g_free(group_name); > > trace_vfio_realize(vdev->vbasedev.name, groupid); > > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c > index 0d4bc0a..15dbae8 100644 > --- a/hw/vfio/platform.c > +++ b/hw/vfio/platform.c > @@ -561,7 +561,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) > /* @sysfsdev takes precedence over @host */ > if (vbasedev->sysfsdev) { > g_free(vbasedev->name); > - vbasedev->name = g_strdup(basename(vbasedev->sysfsdev)); > + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); > } else { > if (!vbasedev->name || strchr(vbasedev->name, '/')) { > error_setg(errp, "wrong host device name"); > @@ -590,11 +590,13 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) > > group_path[len] = 0; > > - group_name = basename(group_path); > + group_name = g_path_get_basename(group_path); > if (sscanf(group_name, "%d", &groupid) != 1) { > error_setg_errno(errp, errno, "failed to read %s", group_path); > + g_free(group_name); > return -errno; > } > + g_free(group_name); > > trace_vfio_platform_base_device_init(vbasedev->name, groupid); > > diff --git a/qemu-io.c b/qemu-io.c > index 2c00ea0..1de5fc7 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -308,6 +308,11 @@ static char *get_prompt(void) > return prompt; > } > > +static void free_progname(void) > +{ > + g_free(progname); > +} > + > static void GCC_FMT_ATTR(2, 3) readline_printf_func(void *opaque, > const char *fmt, ...) > { > @@ -504,7 +509,8 @@ int main(int argc, char **argv) > #endif > > module_call_init(MODULE_INIT_TRACE); > - progname = basename(argv[0]); > + progname = g_path_get_basename(argv[0]); > + atexit(free_progname); > qemu_init_exec_dir(argv[0]); > > qcrypto_init(&error_fatal); > diff --git a/qemu-nbd.c b/qemu-nbd.c > index ed5d9b5..36bafe7 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -893,8 +893,11 @@ int main(int argc, char **argv) > } > > if (device != NULL && sockpath == NULL) { > + char *base_filename = g_path_get_basename(device); > + > sockpath = g_malloc(128); > - snprintf(sockpath, 128, SOCKET_PATH, basename(device)); > + snprintf(sockpath, 128, SOCKET_PATH, base_filename); > + g_free(base_filename); > } > > server = qio_net_listener_new(); > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 9670614..53a29e3 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -808,7 +808,7 @@ static char *get_pci_driver(char const *syspath, int pathlen, Error **errp) > len = readlink(dpath, buf, sizeof(buf) - 1); > if (len != -1) { > buf[len] = 0; > - driver = g_strdup(basename(buf)); > + driver = g_path_get_basename(buf); > } > g_free(dpath); > g_free(path); > @@ -1053,7 +1053,7 @@ static void build_guest_fsinfo_for_device(char const *devpath, > } > > if (!fs->name) { > - fs->name = g_strdup(basename(syspath)); > + fs->name = g_path_get_basename(syspath); > } > > g_debug(" parse sysfs path '%s'", syspath); > -- > 2.1.4 > >
On Thu, 1 Mar 2018 10:47:42 +0100 Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > Hi > > On Thu, Mar 1, 2018 at 8:08 AM, Julia Suvorova via Qemu-devel > <qemu-devel@nongnu.org> wrote: > > basename(3) and dirname(3) modify their argument and may return > > pointers to statically allocated memory which may be overwritten by > > subsequent calls. > > g_path_get_basename and g_path_get_dirname have no such issues, and > > therefore more preferable. > > > > Signed-off-by: Julia Suvorova <jusual@mail.ru> > > What about adding a warning for basename()/dirname() usage in > scripts/checkpatch.pl ? +1 to that. > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 01/03/2018 08:08, Julia Suvorova wrote: > +static void free_progname(void) > +{ > + g_free(progname); > +} > + > static void GCC_FMT_ATTR(2, 3) readline_printf_func(void *opaque, > const char *fmt, ...) > { > @@ -504,7 +509,8 @@ int main(int argc, char **argv) > #endif > > module_call_init(MODULE_INIT_TRACE); > - progname = basename(argv[0]); > + progname = g_path_get_basename(argv[0]); > + atexit(free_progname); This atexit is not needed; memory is always freed on exit. But, it's good that you thought about it! I can queue this patch for merging once I get an ack from Alex Williamson (VFIO). Thanks, Paolo
On 01/03/2018 11:59, Cornelia Huck wrote: >>> >>> Signed-off-by: Julia Suvorova <jusual@mail.ru> >> What about adding a warning for basename()/dirname() usage in >> scripts/checkpatch.pl ? > +1 to that. > Good idea indeed. Julia, would you like to send a second patch that adds the warning? There are already some examples (__FUNCTION__, strto*, signal, etc.) that you can take inspiration from. Paolo
On 01.03.2018 13:59, Cornelia Huck wrote: > On Thu, 1 Mar 2018 10:47:42 +0100 > Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > >> Hi >> >> On Thu, Mar 1, 2018 at 8:08 AM, Julia Suvorova via Qemu-devel >> <qemu-devel@nongnu.org> wrote: >>> basename(3) and dirname(3) modify their argument and may return >>> pointers to statically allocated memory which may be overwritten by >>> subsequent calls. >>> g_path_get_basename and g_path_get_dirname have no such issues, and >>> therefore more preferable. >>> >>> Signed-off-by: Julia Suvorova <jusual@mail.ru> >> >> What about adding a warning for basename()/dirname() usage in >> scripts/checkpatch.pl ? > > +1 to that. > Ok. I'll prepare a separate patch for that. Thanks for the review. >> >> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > Reviewed-by: Cornelia Huck <cohuck@redhat.com> >
On 01.03.2018 14:21, Paolo Bonzini wrote: > On 01/03/2018 11:59, Cornelia Huck wrote: >>>> >>>> Signed-off-by: Julia Suvorova <jusual@mail.ru> >>> What about adding a warning for basename()/dirname() usage in >>> scripts/checkpatch.pl ? >> +1 to that. >> > > Good idea indeed. Julia, would you like to send a second patch that > adds the warning? > Ok, sure. > There are already some examples (__FUNCTION__, strto*, signal, etc.) > that you can take inspiration from. > Thanks for the tips.
On Thu, 1 Mar 2018 10:08:06 +0300 Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> wrote: > basename(3) and dirname(3) modify their argument and may return > pointers to statically allocated memory which may be overwritten by > subsequent calls. > g_path_get_basename and g_path_get_dirname have no such issues, and > therefore more preferable. I think it's quite a bit arguable whether it's preferable, afaict there are no bugs fixed here. The basic functions are being used correctly and are more conservative of memory usage than the glib variants. In several cases below the basic functions seem far more efficient and we don't need to worry about freeing unnecessarily allocated memory, the diffstat seems to attest to this. Is the inefficiency and possibility of leaking unintentionally allocated memory a statistical improvement over the claimed perils of these absolutely standard and well known functions? I know mine is a losing battle against glib, but the changelog might as well just say "Succumb to glib" because I fail to see that there's actually an improvement here. If Paolo wants to take this, Acked-by: Alex Williamson <alex.williamson@redhat.com> Thanks, Alex > Signed-off-by: Julia Suvorova <jusual@mail.ru> > --- > fsdev/virtfs-proxy-helper.c | 6 +++++- > hw/s390x/s390-ccw.c | 17 +++++++++++------ > hw/vfio/ccw.c | 7 +++++-- > hw/vfio/pci.c | 6 ++++-- > hw/vfio/platform.c | 6 ++++-- > qemu-io.c | 8 +++++++- > qemu-nbd.c | 5 ++++- > qga/commands-posix.c | 4 ++-- > 8 files changed, 42 insertions(+), 17 deletions(-) > > diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c > index 8e48500..da3452f 100644 > --- a/fsdev/virtfs-proxy-helper.c > +++ b/fsdev/virtfs-proxy-helper.c > @@ -787,6 +787,8 @@ error: > > static void usage(char *prog) > { > + char *base_filename = g_path_get_basename(prog); > + > fprintf(stderr, "usage: %s\n" > " -p|--path <path> 9p path to export\n" > " {-f|--fd <socket-descriptor>} socket file descriptor to be used\n" > @@ -795,7 +797,9 @@ static void usage(char *prog) > " access to this socket\n" > " \tNote: -s & -f can not be used together\n" > " [-n|--nodaemon] Run as a normal program\n", > - basename(prog)); > + base_filename); > + > + g_free(base_filename); > } > > static int process_reply(int sock, int type, > diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c > index 7fc1c60..460dbab 100644 > --- a/hw/s390x/s390-ccw.c > +++ b/hw/s390x/s390-ccw.c > @@ -34,7 +34,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev, > Error **errp) > { > unsigned int cssid, ssid, devid; > - char dev_path[PATH_MAX] = {0}, *tmp; > + char dev_path[PATH_MAX] = {0}, *dir_name, *dir_path; > > if (!sysfsdev) { > error_setg(errp, "No host device provided"); > @@ -48,18 +48,23 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev, > return; > } > > - cdev->mdevid = g_strdup(basename(dev_path)); > + cdev->mdevid = g_path_get_basename(dev_path); > > - tmp = basename(dirname(dev_path)); > - if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) { > - error_setg_errno(errp, errno, "Failed to read %s", tmp); > - return; > + dir_path = g_path_get_dirname(dev_path); > + dir_name = g_path_get_basename(dir_path); > + if (sscanf(dir_name, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) { > + error_setg_errno(errp, errno, "Failed to read %s", dir_name); > + goto out; > } > > cdev->hostid.cssid = cssid; > cdev->hostid.ssid = ssid; > cdev->hostid.devid = devid; > cdev->hostid.valid = true; > + > +out: > + g_free(dir_path); > + g_free(dir_name); > } > > static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp) > diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c > index 16713f2..c0566a9 100644 > --- a/hw/vfio/ccw.c > +++ b/hw/vfio/ccw.c > @@ -300,7 +300,7 @@ static void vfio_put_device(VFIOCCWDevice *vcdev) > > static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp) > { > - char *tmp, group_path[PATH_MAX]; > + char *tmp, *group_name, group_path[PATH_MAX]; > ssize_t len; > int groupid; > > @@ -317,10 +317,13 @@ static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp) > > group_path[len] = 0; > > - if (sscanf(basename(group_path), "%d", &groupid) != 1) { > + group_name = g_path_get_basename(group_path); > + if (sscanf(group_name, "%d", &groupid) != 1) { > error_setg(errp, "vfio: failed to read %s", group_path); > + g_free(group_name); > return NULL; > } > + g_free(group_name); > > return vfio_get_group(groupid, &address_space_memory, errp); > } > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 033cc8d..ba03136 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2807,7 +2807,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > return; > } > > - vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev)); > + vdev->vbasedev.name = g_path_get_basename(vdev->vbasedev.sysfsdev); > vdev->vbasedev.ops = &vfio_pci_ops; > vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI; > vdev->vbasedev.dev = &vdev->pdev.qdev; > @@ -2824,11 +2824,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) > > group_path[len] = 0; > > - group_name = basename(group_path); > + group_name = g_path_get_basename(group_path); > if (sscanf(group_name, "%d", &groupid) != 1) { > error_setg_errno(errp, errno, "failed to read %s", group_path); > + g_free(group_name); > goto error; > } > + g_free(group_name); > > trace_vfio_realize(vdev->vbasedev.name, groupid); > > diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c > index 0d4bc0a..15dbae8 100644 > --- a/hw/vfio/platform.c > +++ b/hw/vfio/platform.c > @@ -561,7 +561,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) > /* @sysfsdev takes precedence over @host */ > if (vbasedev->sysfsdev) { > g_free(vbasedev->name); > - vbasedev->name = g_strdup(basename(vbasedev->sysfsdev)); > + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); > } else { > if (!vbasedev->name || strchr(vbasedev->name, '/')) { > error_setg(errp, "wrong host device name"); > @@ -590,11 +590,13 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) > > group_path[len] = 0; > > - group_name = basename(group_path); > + group_name = g_path_get_basename(group_path); > if (sscanf(group_name, "%d", &groupid) != 1) { > error_setg_errno(errp, errno, "failed to read %s", group_path); > + g_free(group_name); > return -errno; > } > + g_free(group_name); > > trace_vfio_platform_base_device_init(vbasedev->name, groupid); > > diff --git a/qemu-io.c b/qemu-io.c > index 2c00ea0..1de5fc7 100644 > --- a/qemu-io.c > +++ b/qemu-io.c > @@ -308,6 +308,11 @@ static char *get_prompt(void) > return prompt; > } > > +static void free_progname(void) > +{ > + g_free(progname); > +} > + > static void GCC_FMT_ATTR(2, 3) readline_printf_func(void *opaque, > const char *fmt, ...) > { > @@ -504,7 +509,8 @@ int main(int argc, char **argv) > #endif > > module_call_init(MODULE_INIT_TRACE); > - progname = basename(argv[0]); > + progname = g_path_get_basename(argv[0]); > + atexit(free_progname); > qemu_init_exec_dir(argv[0]); > > qcrypto_init(&error_fatal); > diff --git a/qemu-nbd.c b/qemu-nbd.c > index ed5d9b5..36bafe7 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -893,8 +893,11 @@ int main(int argc, char **argv) > } > > if (device != NULL && sockpath == NULL) { > + char *base_filename = g_path_get_basename(device); > + > sockpath = g_malloc(128); > - snprintf(sockpath, 128, SOCKET_PATH, basename(device)); > + snprintf(sockpath, 128, SOCKET_PATH, base_filename); > + g_free(base_filename); > } > > server = qio_net_listener_new(); > diff --git a/qga/commands-posix.c b/qga/commands-posix.c > index 9670614..53a29e3 100644 > --- a/qga/commands-posix.c > +++ b/qga/commands-posix.c > @@ -808,7 +808,7 @@ static char *get_pci_driver(char const *syspath, int pathlen, Error **errp) > len = readlink(dpath, buf, sizeof(buf) - 1); > if (len != -1) { > buf[len] = 0; > - driver = g_strdup(basename(buf)); > + driver = g_path_get_basename(buf); > } > g_free(dpath); > g_free(path); > @@ -1053,7 +1053,7 @@ static void build_guest_fsinfo_for_device(char const *devpath, > } > > if (!fs->name) { > - fs->name = g_strdup(basename(syspath)); > + fs->name = g_path_get_basename(syspath); > } > > g_debug(" parse sysfs path '%s'", syspath);
On 01/03/2018 17:20, Alex Williamson wrote: >> basename(3) and dirname(3) modify their argument and may return >> pointers to statically allocated memory which may be overwritten by >> subsequent calls. >> g_path_get_basename and g_path_get_dirname have no such issues, and >> therefore more preferable. > > I think it's quite a bit arguable whether it's preferable, afaict there > are no bugs fixed here. The basic functions are being used correctly > and are more conservative of memory usage than the glib variants. In > several cases below the basic functions seem far more efficient and we > don't need to worry about freeing unnecessarily allocated memory, the > diffstat seems to attest to this. Is the inefficiency and possibility > of leaking unintentionally allocated memory a statistical improvement > over the claimed perils of these absolutely standard and well known > functions? > > I know mine is a losing battle against glib, but the changelog might as > well just say "Succumb to glib" because I fail to see that there's > actually an improvement here. If Paolo wants to take this, > > Acked-by: Alex Williamson <alex.williamson@redhat.com> Fair enough, I'll apply only the obvious cases like g_strdup(basename()). Paolo
On 03/01/2018 10:20 AM, Alex Williamson wrote: > On Thu, 1 Mar 2018 10:08:06 +0300 > Julia Suvorova via Qemu-devel <qemu-devel@nongnu.org> wrote: > >> basename(3) and dirname(3) modify their argument and may return >> pointers to statically allocated memory which may be overwritten by >> subsequent calls. >> g_path_get_basename and g_path_get_dirname have no such issues, and >> therefore more preferable. > > I think it's quite a bit arguable whether it's preferable, afaict there > are no bugs fixed here. The basic functions are being used correctly > and are more conservative of memory usage than the glib variants. In > several cases below the basic functions seem far more efficient and we > don't need to worry about freeing unnecessarily allocated memory, the > diffstat seems to attest to this. Is the inefficiency and possibility > of leaking unintentionally allocated memory a statistical improvement > over the claimed perils of these absolutely standard and well known > functions? The standard functions may be well-known, but as currently standardized are not very reliably useful in a multi-threaded program. However, the standard WILL be improved in the future to require a saner standardized implementation (the proposed changes to the standard remove the ability for basename()/dirname() to return NULL, remove the allowance for non-thread-safe returns, and document more fully that the returned pointer will be back to the input string in all but a few corner cases which return const char *): http://austingroupbugs.net/view.php?id=1064 > > I know mine is a losing battle against glib, but the changelog might as > well just say "Succumb to glib" because I fail to see that there's > actually an improvement here. If Paolo wants to take this, One thing that the glib functions can do that the standardized version cannot do is handle drive letters on DOS-like systems (doesn't affect our builds on Linux or BSD, but may matter to mingw builds). But you are right that the glib functions are more malloc-heavy than the (fixed) standardized functions, so it becomes a trade-off of how likely the libc functions are to comply with the tighter future standard version (that is actually somewhat useful) vs. the current standard version (that is a piece of trash because it has too many loopholes).
diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 8e48500..da3452f 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -787,6 +787,8 @@ error: static void usage(char *prog) { + char *base_filename = g_path_get_basename(prog); + fprintf(stderr, "usage: %s\n" " -p|--path <path> 9p path to export\n" " {-f|--fd <socket-descriptor>} socket file descriptor to be used\n" @@ -795,7 +797,9 @@ static void usage(char *prog) " access to this socket\n" " \tNote: -s & -f can not be used together\n" " [-n|--nodaemon] Run as a normal program\n", - basename(prog)); + base_filename); + + g_free(base_filename); } static int process_reply(int sock, int type, diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c index 7fc1c60..460dbab 100644 --- a/hw/s390x/s390-ccw.c +++ b/hw/s390x/s390-ccw.c @@ -34,7 +34,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev, Error **errp) { unsigned int cssid, ssid, devid; - char dev_path[PATH_MAX] = {0}, *tmp; + char dev_path[PATH_MAX] = {0}, *dir_name, *dir_path; if (!sysfsdev) { error_setg(errp, "No host device provided"); @@ -48,18 +48,23 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev, return; } - cdev->mdevid = g_strdup(basename(dev_path)); + cdev->mdevid = g_path_get_basename(dev_path); - tmp = basename(dirname(dev_path)); - if (sscanf(tmp, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) { - error_setg_errno(errp, errno, "Failed to read %s", tmp); - return; + dir_path = g_path_get_dirname(dev_path); + dir_name = g_path_get_basename(dir_path); + if (sscanf(dir_name, "%2x.%1x.%4x", &cssid, &ssid, &devid) != 3) { + error_setg_errno(errp, errno, "Failed to read %s", dir_name); + goto out; } cdev->hostid.cssid = cssid; cdev->hostid.ssid = ssid; cdev->hostid.devid = devid; cdev->hostid.valid = true; + +out: + g_free(dir_path); + g_free(dir_name); } static void s390_ccw_realize(S390CCWDevice *cdev, char *sysfsdev, Error **errp) diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c index 16713f2..c0566a9 100644 --- a/hw/vfio/ccw.c +++ b/hw/vfio/ccw.c @@ -300,7 +300,7 @@ static void vfio_put_device(VFIOCCWDevice *vcdev) static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp) { - char *tmp, group_path[PATH_MAX]; + char *tmp, *group_name, group_path[PATH_MAX]; ssize_t len; int groupid; @@ -317,10 +317,13 @@ static VFIOGroup *vfio_ccw_get_group(S390CCWDevice *cdev, Error **errp) group_path[len] = 0; - if (sscanf(basename(group_path), "%d", &groupid) != 1) { + group_name = g_path_get_basename(group_path); + if (sscanf(group_name, "%d", &groupid) != 1) { error_setg(errp, "vfio: failed to read %s", group_path); + g_free(group_name); return NULL; } + g_free(group_name); return vfio_get_group(groupid, &address_space_memory, errp); } diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index 033cc8d..ba03136 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2807,7 +2807,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) return; } - vdev->vbasedev.name = g_strdup(basename(vdev->vbasedev.sysfsdev)); + vdev->vbasedev.name = g_path_get_basename(vdev->vbasedev.sysfsdev); vdev->vbasedev.ops = &vfio_pci_ops; vdev->vbasedev.type = VFIO_DEVICE_TYPE_PCI; vdev->vbasedev.dev = &vdev->pdev.qdev; @@ -2824,11 +2824,13 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) group_path[len] = 0; - group_name = basename(group_path); + group_name = g_path_get_basename(group_path); if (sscanf(group_name, "%d", &groupid) != 1) { error_setg_errno(errp, errno, "failed to read %s", group_path); + g_free(group_name); goto error; } + g_free(group_name); trace_vfio_realize(vdev->vbasedev.name, groupid); diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c index 0d4bc0a..15dbae8 100644 --- a/hw/vfio/platform.c +++ b/hw/vfio/platform.c @@ -561,7 +561,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) /* @sysfsdev takes precedence over @host */ if (vbasedev->sysfsdev) { g_free(vbasedev->name); - vbasedev->name = g_strdup(basename(vbasedev->sysfsdev)); + vbasedev->name = g_path_get_basename(vbasedev->sysfsdev); } else { if (!vbasedev->name || strchr(vbasedev->name, '/')) { error_setg(errp, "wrong host device name"); @@ -590,11 +590,13 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp) group_path[len] = 0; - group_name = basename(group_path); + group_name = g_path_get_basename(group_path); if (sscanf(group_name, "%d", &groupid) != 1) { error_setg_errno(errp, errno, "failed to read %s", group_path); + g_free(group_name); return -errno; } + g_free(group_name); trace_vfio_platform_base_device_init(vbasedev->name, groupid); diff --git a/qemu-io.c b/qemu-io.c index 2c00ea0..1de5fc7 100644 --- a/qemu-io.c +++ b/qemu-io.c @@ -308,6 +308,11 @@ static char *get_prompt(void) return prompt; } +static void free_progname(void) +{ + g_free(progname); +} + static void GCC_FMT_ATTR(2, 3) readline_printf_func(void *opaque, const char *fmt, ...) { @@ -504,7 +509,8 @@ int main(int argc, char **argv) #endif module_call_init(MODULE_INIT_TRACE); - progname = basename(argv[0]); + progname = g_path_get_basename(argv[0]); + atexit(free_progname); qemu_init_exec_dir(argv[0]); qcrypto_init(&error_fatal); diff --git a/qemu-nbd.c b/qemu-nbd.c index ed5d9b5..36bafe7 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -893,8 +893,11 @@ int main(int argc, char **argv) } if (device != NULL && sockpath == NULL) { + char *base_filename = g_path_get_basename(device); + sockpath = g_malloc(128); - snprintf(sockpath, 128, SOCKET_PATH, basename(device)); + snprintf(sockpath, 128, SOCKET_PATH, base_filename); + g_free(base_filename); } server = qio_net_listener_new(); diff --git a/qga/commands-posix.c b/qga/commands-posix.c index 9670614..53a29e3 100644 --- a/qga/commands-posix.c +++ b/qga/commands-posix.c @@ -808,7 +808,7 @@ static char *get_pci_driver(char const *syspath, int pathlen, Error **errp) len = readlink(dpath, buf, sizeof(buf) - 1); if (len != -1) { buf[len] = 0; - driver = g_strdup(basename(buf)); + driver = g_path_get_basename(buf); } g_free(dpath); g_free(path); @@ -1053,7 +1053,7 @@ static void build_guest_fsinfo_for_device(char const *devpath, } if (!fs->name) { - fs->name = g_strdup(basename(syspath)); + fs->name = g_path_get_basename(syspath); } g_debug(" parse sysfs path '%s'", syspath);
basename(3) and dirname(3) modify their argument and may return pointers to statically allocated memory which may be overwritten by subsequent calls. g_path_get_basename and g_path_get_dirname have no such issues, and therefore more preferable. Signed-off-by: Julia Suvorova <jusual@mail.ru> --- fsdev/virtfs-proxy-helper.c | 6 +++++- hw/s390x/s390-ccw.c | 17 +++++++++++------ hw/vfio/ccw.c | 7 +++++-- hw/vfio/pci.c | 6 ++++-- hw/vfio/platform.c | 6 ++++-- qemu-io.c | 8 +++++++- qemu-nbd.c | 5 ++++- qga/commands-posix.c | 4 ++-- 8 files changed, 42 insertions(+), 17 deletions(-)