| Message ID | 1457420446-25276-6-git-send-email-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8 March 2016 at 14:00, Peter Xu <peterx@redhat.com> wrote: > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > CC: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/usb/dev-mtp.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c > index 7391783..e6dae2f 100644 > --- a/hw/usb/dev-mtp.c > +++ b/hw/usb/dev-mtp.c > @@ -432,13 +432,13 @@ static void inotify_watchfn(void *arg) > { > MTPState *s = arg; > ssize_t bytes; > +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1) > /* From the man page: atleast one event can be read */ > - int len = sizeof(struct inotify_event) + NAME_MAX + 1; > int pos; > - char buf[len]; > + char buf[__BUF_LEN]; The commit message subject says this is fixing an unbounded stack usage, but (a) this array wasn't unbounded in size (b) the change doesn't change the size we allocate. What are you trying to do here? thanks -- PMM
On 08/03/2016 08:00, Peter Xu wrote: > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > CC: Gerd Hoffmann <kraxel@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/usb/dev-mtp.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c > index 7391783..e6dae2f 100644 > --- a/hw/usb/dev-mtp.c > +++ b/hw/usb/dev-mtp.c > @@ -432,13 +432,13 @@ static void inotify_watchfn(void *arg) > { > MTPState *s = arg; > ssize_t bytes; > +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1) > /* From the man page: atleast one event can be read */ > - int len = sizeof(struct inotify_event) + NAME_MAX + 1; > int pos; > - char buf[len]; > + char buf[__BUF_LEN]; > > for (;;) { > - bytes = read(s->inotifyfd, buf, len); > + bytes = read(s->inotifyfd, buf, __BUF_LEN); Again, here you can use ARRAY_SIZE(buf) and avoid the macro. Paolo > pos = 0; > > if (bytes <= 0) { > @@ -534,6 +534,7 @@ static void inotify_watchfn(void *arg) > } > } > } > +#undef __BUF_LEN > } > > static int usb_mtp_inotify_init(MTPState *s) >
On 08/03/2016 08:20, Peter Maydell wrote: >> > +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1) >> > /* From the man page: atleast one event can be read */ >> > - int len = sizeof(struct inotify_event) + NAME_MAX + 1; >> > int pos; >> > - char buf[len]; >> > + char buf[__BUF_LEN]; > The commit message subject says this is fixing an unbounded > stack usage, but (a) this array wasn't unbounded in size > (b) the change doesn't change the size we allocate. > What are you trying to do here? I suspect it's just fixing a false positive in the compiler. Paolo
On Tue, Mar 08, 2016 at 01:22:46PM +0100, Paolo Bonzini wrote: > > > On 08/03/2016 08:20, Peter Maydell wrote: > >> > +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1) > >> > /* From the man page: atleast one event can be read */ > >> > - int len = sizeof(struct inotify_event) + NAME_MAX + 1; > >> > int pos; > >> > - char buf[len]; > >> > + char buf[__BUF_LEN]; > > The commit message subject says this is fixing an unbounded > > stack usage, but (a) this array wasn't unbounded in size > > (b) the change doesn't change the size we allocate. > > What are you trying to do here? Sorry. I should be more clear to say "it avoids one warning during compilation" rather than saying "fix unbounded stack usage", while it's not. > > I suspect it's just fixing a false positive in the compiler. > > Paolo Yes. I will avoid touching these kinds of places in the code next time I guess... only when necessary. Since this one is easy, I'd like to send another standalone patch, using sizeof(). rather than macros, to avoid the warning. Thanks. Peter
On Tue, Mar 08, 2016 at 01:22:19PM +0100, Paolo Bonzini wrote: > > for (;;) { > > - bytes = read(s->inotifyfd, buf, len); > > + bytes = read(s->inotifyfd, buf, __BUF_LEN); > > Again, here you can use ARRAY_SIZE(buf) and avoid the macro. Yes, will fix. Thanks! Peter
diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 7391783..e6dae2f 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -432,13 +432,13 @@ static void inotify_watchfn(void *arg) { MTPState *s = arg; ssize_t bytes; +#define __BUF_LEN (sizeof(struct inotify_event) + NAME_MAX + 1) /* From the man page: atleast one event can be read */ - int len = sizeof(struct inotify_event) + NAME_MAX + 1; int pos; - char buf[len]; + char buf[__BUF_LEN]; for (;;) { - bytes = read(s->inotifyfd, buf, len); + bytes = read(s->inotifyfd, buf, __BUF_LEN); pos = 0; if (bytes <= 0) { @@ -534,6 +534,7 @@ static void inotify_watchfn(void *arg) } } } +#undef __BUF_LEN } static int usb_mtp_inotify_init(MTPState *s)
Suggested-by: Paolo Bonzini <pbonzini@redhat.com> CC: Gerd Hoffmann <kraxel@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/usb/dev-mtp.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)