[PATCH] util/vfio-helpers: Use g_file_read_link()

[PATCH] util/vfio-helpers: Use g_file_read_link()

[Akihiko Odaki]

When _FORTIFY_SOURCE=2, glibc version is 2.35, and GCC version is
12.1.0, the compiler complains as follows:

In file included from /usr/include/features.h:490,
                 from /usr/include/bits/libc-header-start.h:33,
                 from /usr/include/stdint.h:26,
                 from /usr/lib/gcc/aarch64-unknown-linux-gnu/12.1.0/include/stdint.h:9,
                 from /home/alarm/q/var/qemu/include/qemu/osdep.h:94,
                 from ../util/vfio-helpers.c:13:
In function 'readlink',
    inlined from 'sysfs_find_group_file' at ../util/vfio-helpers.c:116:9,
    inlined from 'qemu_vfio_init_pci' at ../util/vfio-helpers.c:326:18,
    inlined from 'qemu_vfio_open_pci' at ../util/vfio-helpers.c:517:9:
/usr/include/bits/unistd.h:119:10: error: argument 2 is null but the corresponding size argument 3 value is 4095 [-Werror=nonnull]
  119 |   return __glibc_fortify (readlink, __len, sizeof (char),
      |          ^~~~~~~~~~~~~~~

This error implies the allocated buffer can be NULL. Use
g_file_read_link(), which allocates buffer automatically to avoid the
error.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 util/vfio-helpers.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
index 2d8af38f88..e482ab22e2 100644
--- a/util/vfio-helpers.c
+++ b/util/vfio-helpers.c
@@ -106,15 +106,17 @@ struct QEMUVFIOState {
  */
 static char *sysfs_find_group_file(const char *device, Error **errp)
 {
+    g_autoptr(GError) gerr;
     char *sysfs_link;
     char *sysfs_group;
     char *p;
     char *path = NULL;
 
     sysfs_link = g_strdup_printf("/sys/bus/pci/devices/%s/iommu_group", device);
-    sysfs_group = g_malloc0(PATH_MAX);
-    if (readlink(sysfs_link, sysfs_group, PATH_MAX - 1) == -1) {
-        error_setg_errno(errp, errno, "Failed to find iommu group sysfs path");
+    sysfs_group = g_file_read_link(sysfs_link, &gerr);
+    if (gerr) {
+        error_setg(errp, "Failed to find iommu group sysfs path: %s",
+                   gerr->message);
         goto out;
     }
     p = strrchr(sysfs_group, '/');
-- 
2.40.1

Re: [PATCH] util/vfio-helpers: Use g_file_read_link()

[Philippe Mathieu-Daudé]

On 22/5/23 13:49, Akihiko Odaki wrote:
> When _FORTIFY_SOURCE=2, glibc version is 2.35, and GCC version is
> 12.1.0, the compiler complains as follows:
> 
> In file included from /usr/include/features.h:490,
>                   from /usr/include/bits/libc-header-start.h:33,
>                   from /usr/include/stdint.h:26,
>                   from /usr/lib/gcc/aarch64-unknown-linux-gnu/12.1.0/include/stdint.h:9,
>                   from /home/alarm/q/var/qemu/include/qemu/osdep.h:94,
>                   from ../util/vfio-helpers.c:13:
> In function 'readlink',
>      inlined from 'sysfs_find_group_file' at ../util/vfio-helpers.c:116:9,
>      inlined from 'qemu_vfio_init_pci' at ../util/vfio-helpers.c:326:18,
>      inlined from 'qemu_vfio_open_pci' at ../util/vfio-helpers.c:517:9:
> /usr/include/bits/unistd.h:119:10: error: argument 2 is null but the corresponding size argument 3 value is 4095 [-Werror=nonnull]
>    119 |   return __glibc_fortify (readlink, __len, sizeof (char),
>        |          ^~~~~~~~~~~~~~~
> 
> This error implies the allocated buffer can be NULL. Use
> g_file_read_link(), which allocates buffer automatically to avoid the
> error.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   util/vfio-helpers.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 2d8af38f88..e482ab22e2 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -106,15 +106,17 @@ struct QEMUVFIOState {
>    */
>   static char *sysfs_find_group_file(const char *device, Error **errp)
>   {
> +    g_autoptr(GError) gerr;

Shouldn't this also be NULL-initialized (other picky compilers)?

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>       char *sysfs_link;
>       char *sysfs_group;
>       char *p;
>       char *path = NULL;
>   
>       sysfs_link = g_strdup_printf("/sys/bus/pci/devices/%s/iommu_group", device);
> -    sysfs_group = g_malloc0(PATH_MAX);
> -    if (readlink(sysfs_link, sysfs_group, PATH_MAX - 1) == -1) {
> -        error_setg_errno(errp, errno, "Failed to find iommu group sysfs path");
> +    sysfs_group = g_file_read_link(sysfs_link, &gerr);
> +    if (gerr) {
> +        error_setg(errp, "Failed to find iommu group sysfs path: %s",
> +                   gerr->message);
>           goto out;
>       }
>       p = strrchr(sysfs_group, '/');

Re: [PATCH] util/vfio-helpers: Use g_file_read_link()

[Daniel P. Berrangé]

On Mon, May 22, 2023 at 02:38:44PM +0200, Philippe Mathieu-Daudé wrote:
> On 22/5/23 13:49, Akihiko Odaki wrote:
> > When _FORTIFY_SOURCE=2, glibc version is 2.35, and GCC version is
> > 12.1.0, the compiler complains as follows:
> > 
> > In file included from /usr/include/features.h:490,
> >                   from /usr/include/bits/libc-header-start.h:33,
> >                   from /usr/include/stdint.h:26,
> >                   from /usr/lib/gcc/aarch64-unknown-linux-gnu/12.1.0/include/stdint.h:9,
> >                   from /home/alarm/q/var/qemu/include/qemu/osdep.h:94,
> >                   from ../util/vfio-helpers.c:13:
> > In function 'readlink',
> >      inlined from 'sysfs_find_group_file' at ../util/vfio-helpers.c:116:9,
> >      inlined from 'qemu_vfio_init_pci' at ../util/vfio-helpers.c:326:18,
> >      inlined from 'qemu_vfio_open_pci' at ../util/vfio-helpers.c:517:9:
> > /usr/include/bits/unistd.h:119:10: error: argument 2 is null but the corresponding size argument 3 value is 4095 [-Werror=nonnull]
> >    119 |   return __glibc_fortify (readlink, __len, sizeof (char),
> >        |          ^~~~~~~~~~~~~~~
> > 
> > This error implies the allocated buffer can be NULL. Use
> > g_file_read_link(), which allocates buffer automatically to avoid the
> > error.
> > 
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   util/vfio-helpers.c | 8 +++++---
> >   1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> > index 2d8af38f88..e482ab22e2 100644
> > --- a/util/vfio-helpers.c
> > +++ b/util/vfio-helpers.c
> > @@ -106,15 +106,17 @@ struct QEMUVFIOState {
> >    */
> >   static char *sysfs_find_group_file(const char *device, Error **errp)
> >   {
> > +    g_autoptr(GError) gerr;
> 
> Shouldn't this also be NULL-initialized (other picky compilers)?

Yes, *all* use of g_auto* must have an initializer at time of
declaration.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] util/vfio-helpers: Use g_file_read_link()

[Cédric Le Goater]

Hello Akihiko,

On 5/22/23 13:49, Akihiko Odaki wrote:
> When _FORTIFY_SOURCE=2, glibc version is 2.35, and GCC version is
> 12.1.0, the compiler complains as follows:
> 
> In file included from /usr/include/features.h:490,
>                   from /usr/include/bits/libc-header-start.h:33,
>                   from /usr/include/stdint.h:26,
>                   from /usr/lib/gcc/aarch64-unknown-linux-gnu/12.1.0/include/stdint.h:9,
>                   from /home/alarm/q/var/qemu/include/qemu/osdep.h:94,
>                   from ../util/vfio-helpers.c:13:
> In function 'readlink',
>      inlined from 'sysfs_find_group_file' at ../util/vfio-helpers.c:116:9,
>      inlined from 'qemu_vfio_init_pci' at ../util/vfio-helpers.c:326:18,
>      inlined from 'qemu_vfio_open_pci' at ../util/vfio-helpers.c:517:9:
> /usr/include/bits/unistd.h:119:10: error: argument 2 is null but the corresponding size argument 3 value is 4095 [-Werror=nonnull]
>    119 |   return __glibc_fortify (readlink, __len, sizeof (char),
>        |          ^~~~~~~~~~~~~~~
> 
> This error implies the allocated buffer can be NULL. Use
> g_file_read_link(), which allocates buffer automatically to avoid the
> error.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

There are a few other VFIO routines computing the IOMMU group from sysfs :

   vfio_ap_get_group()
   vfio_ccw_get_group()
   vfio_realize()

May be there is a possible common routine to introduce. Anyhow, this is
beyond the scope of this fix

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

> ---
>   util/vfio-helpers.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/util/vfio-helpers.c b/util/vfio-helpers.c
> index 2d8af38f88..e482ab22e2 100644
> --- a/util/vfio-helpers.c
> +++ b/util/vfio-helpers.c
> @@ -106,15 +106,17 @@ struct QEMUVFIOState {
>    */
>   static char *sysfs_find_group_file(const char *device, Error **errp)
>   {
> +    g_autoptr(GError) gerr;
>       char *sysfs_link;
>       char *sysfs_group;
>       char *p;
>       char *path = NULL;
>   
>       sysfs_link = g_strdup_printf("/sys/bus/pci/devices/%s/iommu_group", device);
> -    sysfs_group = g_malloc0(PATH_MAX);
> -    if (readlink(sysfs_link, sysfs_group, PATH_MAX - 1) == -1) {
> -        error_setg_errno(errp, errno, "Failed to find iommu group sysfs path");
> +    sysfs_group = g_file_read_link(sysfs_link, &gerr);
> +    if (gerr) {
> +        error_setg(errp, "Failed to find iommu group sysfs path: %s",
> +                   gerr->message);
>           goto out;
>       }
>       p = strrchr(sysfs_group, '/');