[Qemu-devel] [PATCH] usb: dev-mtp: close fd in usb_mtp_object_readdir()

[Qemu-devel] [PATCH] usb: dev-mtp: close fd in usb_mtp_object_readdir()

[Li Qiang]

Spotted by Coverity: CID 1397070

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/usb/dev-mtp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index b19b576278..666bafd9e8 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -666,6 +666,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
     }
     dir = fdopendir(fd);
     if (!dir) {
+        close(fd);
         return;
     }
 #ifdef CONFIG_INOTIFY1
@@ -682,6 +683,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
         usb_mtp_add_child(s, o, entry->d_name);
     }
     closedir(dir);
+    close(fd);
 }
 
 /* ----------------------------------------------------------------------- */
-- 
2.17.1

Re: [Qemu-devel] [PATCH] usb: dev-mtp: close fd in usb_mtp_object_readdir()

[Philippe Mathieu-Daudé]

On 1/3/19 2:31 PM, Li Qiang wrote:
> Spotted by Coverity: CID 1397070

Closing a CVE to open a CID :)

Fixes: bab9df35ce

> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/usb/dev-mtp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index b19b576278..666bafd9e8 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -666,6 +666,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
>      }
>      dir = fdopendir(fd);
>      if (!dir) {
> +        close(fd);
>          return;

This or:

           goto cleanup_fd;

>      }
>  #ifdef CONFIG_INOTIFY1
> @@ -682,6 +683,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
>          usb_mtp_add_child(s, o, entry->d_name);
>      }
>      closedir(dir);

And:

cleanup_fd:

> +    close(fd);
>  }
>  
>  /* ----------------------------------------------------------------------- */
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH] usb: dev-mtp: close fd in usb_mtp_object_readdir()

[Gerd Hoffmann]

On Thu, Jan 03, 2019 at 05:31:13AM -0800, Li Qiang wrote:
> Spotted by Coverity: CID 1397070

> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index b19b576278..666bafd9e8 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -666,6 +666,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
>      }
>      dir = fdopendir(fd);
>      if (!dir) {
> +        close(fd);
>          return;
>      }

Ok, clearly a bug.

>  #ifdef CONFIG_INOTIFY1
> @@ -682,6 +683,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
>          usb_mtp_add_child(s, o, entry->d_name);
>      }
>      closedir(dir);
> +    close(fd);

Not fully sure this is correct.

The fdopendir manpage says the app should not use fd any more after
successfully calling fdopendir(), and I assumed that includes calling
close().  But I've seen the Coverity message warning this one too, so
maybe I'm wrong ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] usb: dev-mtp: close fd in usb_mtp_object_readdir()

[Peter Maydell]

On Mon, 7 Jan 2019 at 10:22, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Jan 03, 2019 at 05:31:13AM -0800, Li Qiang wrote:
> > Spotted by Coverity: CID 1397070

> >  #ifdef CONFIG_INOTIFY1
> > @@ -682,6 +683,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
> >          usb_mtp_add_child(s, o, entry->d_name);
> >      }
> >      closedir(dir);
> > +    close(fd);
>
> Not fully sure this is correct.
>
> The fdopendir manpage says the app should not use fd any more after
> successfully calling fdopendir(), and I assumed that includes calling
> close().

Yes. The POSIX spec is clearer:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fdopendir.html
"Upon successful return from fdopendir(), the file descriptor is under
 the control of the system, and if any attempt is made to close the file
 descriptor, or to modify the state of the associated description, other
 than by means of closedir(), readdir(), readdir_r(), rewinddir(), or
 seekdir(), the behavior is undefined. Upon calling closedir() the file
 descriptor shall be closed."

> But I've seen the Coverity message warning this one too, so
> maybe I'm wrong ...

If coverity insists on a close(fd) then that's a coverity bug :-)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] usb: dev-mtp: close fd in usb_mtp_object_readdir()

[Gerd Hoffmann]

On Mon, Jan 07, 2019 at 10:35:30AM +0000, Peter Maydell wrote:
> On Mon, 7 Jan 2019 at 10:22, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > On Thu, Jan 03, 2019 at 05:31:13AM -0800, Li Qiang wrote:
> > > Spotted by Coverity: CID 1397070
> 
> > >  #ifdef CONFIG_INOTIFY1
> > > @@ -682,6 +683,7 @@ static void usb_mtp_object_readdir(MTPState *s, MTPObject *o)
> > >          usb_mtp_add_child(s, o, entry->d_name);
> > >      }
> > >      closedir(dir);
> > > +    close(fd);
> >
> > Not fully sure this is correct.
> >
> > The fdopendir manpage says the app should not use fd any more after
> > successfully calling fdopendir(), and I assumed that includes calling
> > close().
> 
> Yes. The POSIX spec is clearer:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/fdopendir.html
> "Upon successful return from fdopendir(), the file descriptor is under
>  the control of the system, and if any attempt is made to close the file
>  descriptor, or to modify the state of the associated description, other
>  than by means of closedir(), readdir(), readdir_r(), rewinddir(), or
>  seekdir(), the behavior is undefined. Upon calling closedir() the file
>  descriptor shall be closed."

Thanks for looking it up.

Patch added to usb queue, with the second chunk dropped.

cheers,
  Gerd