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
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>
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
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
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
© 2016 - 2024 Red Hat, Inc.