* [PATCH] io_u_qiter: Fix buffer overrun @ 2014-02-13 7:06 Sitsofe Wheeler 2014-02-13 16:17 ` Jens Axboe 0 siblings, 1 reply; 4+ messages in thread From: Sitsofe Wheeler @ 2014-02-13 7:06 UTC (permalink / raw) To: fio@vger.kernel.org In io_u_queue.h the io_u_qiter macro is loops around io_u_queue structures. The problem comes with the end of loop initialisation: i++, io_u = (q)->io_us[i] For example, if io_us consists of one element and i is 0 then after the first iteration is completed i++, io_u = (q)->io_us[i] will access beyond the end of io_us. Fix this by moving io_u initialisation to the expression part of the for loop (yuck). Found by Dr Memory. Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com> --- io_u_queue.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io_u_queue.h b/io_u_queue.h index 4f6e8e6..5b6cad0 100644 --- a/io_u_queue.h +++ b/io_u_queue.h @@ -29,7 +29,7 @@ static inline int io_u_qempty(struct io_u_queue *q) } #define io_u_qiter(q, io_u, i) \ - for (i = 0, io_u = (q)->io_us[0]; i < (q)->nr; i++, io_u = (q)->io_us[i]) + for (i = 0; i < (q)->nr && (io_u = (q)->io_us[i]); i++) int io_u_qinit(struct io_u_queue *q, unsigned int nr); void io_u_qexit(struct io_u_queue *q); -- 1.8.5.3 -- Sitsofe | http://sucs.org/~sits/ ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] io_u_qiter: Fix buffer overrun 2014-02-13 7:06 [PATCH] io_u_qiter: Fix buffer overrun Sitsofe Wheeler @ 2014-02-13 16:17 ` Jens Axboe 2014-02-13 20:05 ` Sitsofe Wheeler 0 siblings, 1 reply; 4+ messages in thread From: Jens Axboe @ 2014-02-13 16:17 UTC (permalink / raw) To: Sitsofe Wheeler; +Cc: fio@vger.kernel.org On Thu, Feb 13 2014, Sitsofe Wheeler wrote: > In io_u_queue.h the io_u_qiter macro is loops around io_u_queue > structures. The problem comes with the end of loop initialisation: > i++, io_u = (q)->io_us[i] > For example, if io_us consists of one element and i is 0 then after the > first iteration is completed i++, io_u = (q)->io_us[i] will access > beyond the end of io_us. > > Fix this by moving io_u initialisation to the expression part of the for > loop (yuck). > > Found by Dr Memory. > > Signed-off-by: Sitsofe Wheeler <sitsofe@yahoo.com> > --- > io_u_queue.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/io_u_queue.h b/io_u_queue.h > index 4f6e8e6..5b6cad0 100644 > --- a/io_u_queue.h > +++ b/io_u_queue.h > @@ -29,7 +29,7 @@ static inline int io_u_qempty(struct io_u_queue *q) > } > > #define io_u_qiter(q, io_u, i) \ > - for (i = 0, io_u = (q)->io_us[0]; i < (q)->nr; i++, io_u = (q)->io_us[i]) > + for (i = 0; i < (q)->nr && (io_u = (q)->io_us[i]); i++) Initially I didn't see the issue, but then I realized that ->io_us is a pointer to the io_u pointer. So it is an issue. The fix isn't super pretty, but it gets rid of the bug, so I'll apply it. It might be nicer to split it into a top and bottom define. -- Jens Axboe ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] io_u_qiter: Fix buffer overrun 2014-02-13 16:17 ` Jens Axboe @ 2014-02-13 20:05 ` Sitsofe Wheeler 2014-02-13 20:34 ` Jens Axboe 0 siblings, 1 reply; 4+ messages in thread From: Sitsofe Wheeler @ 2014-02-13 20:05 UTC (permalink / raw) To: Jens Axboe; +Cc: fio@vger.kernel.org On Thu, Feb 13, 2014 at 09:17:33AM -0700, Jens Axboe wrote: > > Initially I didn't see the issue, but then I realized that ->io_us is a > pointer to the io_u pointer. So it is an issue. The fix isn't super > pretty, but it gets rid of the bug, so I'll apply it. It might be nicer > to split it into a top and bottom define. Can you explain this top and bottom define more - would this let me turn it into a while loop? The only reason for the current abuse was because I couldn't think of a another way to fix it while preserving the macro... -- Sitsofe | http://sucs.org/~sits/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] io_u_qiter: Fix buffer overrun 2014-02-13 20:05 ` Sitsofe Wheeler @ 2014-02-13 20:34 ` Jens Axboe 0 siblings, 0 replies; 4+ messages in thread From: Jens Axboe @ 2014-02-13 20:34 UTC (permalink / raw) To: Sitsofe Wheeler; +Cc: fio@vger.kernel.org On Thu, Feb 13 2014, Sitsofe Wheeler wrote: > On Thu, Feb 13, 2014 at 09:17:33AM -0700, Jens Axboe wrote: > > > > Initially I didn't see the issue, but then I realized that ->io_us is a > > pointer to the io_u pointer. So it is an issue. The fix isn't super > > pretty, but it gets rid of the bug, so I'll apply it. It might be nicer > > to split it into a top and bottom define. > > Can you explain this top and bottom define more - would this let me turn > it into a while loop? > > The only reason for the current abuse was because I couldn't think of a > another way to fix it while preserving the macro... Basically you turn it into a do-while with two macros. Ala the below, completely untested... diff --git a/backend.c b/backend.c index 32bc2652bd0b..e355447d1e3a 100644 --- a/backend.c +++ b/backend.c @@ -255,13 +255,13 @@ static void cleanup_pending_aio(struct thread_data *td) struct io_u *io_u; int i; - io_u_qiter(&td->io_u_all, io_u, i) { + io_u_do_qiter(&td->io_u_all, io_u, i) { if (io_u->flags & IO_U_F_FLIGHT) { r = td->io_ops->cancel(td, io_u); if (!r) put_io_u(td, io_u); } - } + } while_each_io_u(&td->io_u_all, io_u, i); } if (td->cur_depth) diff --git a/engines/posixaio.c b/engines/posixaio.c index 2df26af3848e..3b27fcdfcc34 100644 --- a/engines/posixaio.c +++ b/engines/posixaio.c @@ -111,7 +111,7 @@ static int fio_posixaio_getevents(struct thread_data *td, unsigned int min, restart: memset(suspend_list, 0, sizeof(*suspend_list)); suspend_entries = 0; - io_u_qiter(&td->io_u_all, io_u, i) { + io_u_do_qiter(&td->io_u_all, io_u, i) { int err; if (io_u->seen || !(io_u->flags & IO_U_F_FLIGHT)) @@ -138,7 +138,7 @@ restart: io_u->resid = io_u->xfer_buflen - retval; } else io_u->error = err; - } + } while_each_io_u(&td->io_u_all, io_u, i); if (r >= min) return r; diff --git a/engines/windowsaio.c b/engines/windowsaio.c index 16df74035f18..c6ff27a4408f 100644 --- a/engines/windowsaio.c +++ b/engines/windowsaio.c @@ -275,7 +275,7 @@ static int fio_windowsaio_getevents(struct thread_data *td, unsigned int min, } do { - io_u_qiter(&td->io_u_all, io_u, i) { + io_u_do_qiter(&td->io_u_all, io_u, i) { if (!(io_u->flags & IO_U_F_FLIGHT)) continue; @@ -290,7 +290,7 @@ static int fio_windowsaio_getevents(struct thread_data *td, unsigned int min, if (dequeued >= min) break; - } + } while_each_io_u(&td->io_u_all, io_u, i); if (dequeued < min) { status = WaitForSingleObject(wd->iocomplete_event, mswait); diff --git a/io_u_queue.h b/io_u_queue.h index 5b6cad0ef173..649dcb5ca67c 100644 --- a/io_u_queue.h +++ b/io_u_queue.h @@ -28,8 +28,14 @@ static inline int io_u_qempty(struct io_u_queue *q) return !q->nr; } -#define io_u_qiter(q, io_u, i) \ - for (i = 0; i < (q)->nr && (io_u = (q)->io_us[i]); i++) +#define io_u_do_qiter(q, io_u, i) \ + (i) = 0; \ + do { \ + (io_u) = (q)->io_us[i]; \ + (i)++; \ + +#define while_each_io_u(q, io_u, i) \ + } while ((i) < (q)->nr); \ int io_u_qinit(struct io_u_queue *q, unsigned int nr); void io_u_qexit(struct io_u_queue *q); -- Jens Axboe ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-02-13 20:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-02-13 7:06 [PATCH] io_u_qiter: Fix buffer overrun Sitsofe Wheeler 2014-02-13 16:17 ` Jens Axboe 2014-02-13 20:05 ` Sitsofe Wheeler 2014-02-13 20:34 ` Jens Axboe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox