| Message ID | A612847CFE53224C91B23E3A5B48BAC7B9E316CFDF@xmail3.se.axis.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi Marcus, On Tue, Oct 06, 2015 at 02:13:23PM +0200, Marcus Prebble wrote: > Hi mtd-list, > > Assuming the read() call does not return zero and the result is less > than len, the current implementation will overwrite the data already > read in buf which doesn't seem correct. In addition, this means we might not actually fill up the entire buffer, since 2 or more short read()'s might get us to exit the loop with a cumulative value in 'rd', but only a partially-filled buffer. That could cause a user to try and handle garbage/uninitialized data. > Suggested patch attached (git format-patch) > > -Marcus > From 5dccbbd87604665e896472d52f3351c14c24d2b3 Mon Sep 17 00:00:00 2001 > From: Marcus Prebble <prebble@axis.com> > Date: Mon, 5 Oct 2015 17:32:54 +0200 > Subject: [PATCH] mtd_read: Take the buffer offset into account when reading > > Subsequent calls to read() within the loop will now no longer > overwrite the existing contents of buf. > --- > lib/libmtd.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/libmtd.c b/lib/libmtd.c > index 60b4782..bf6d71f 100644 > --- a/lib/libmtd.c > +++ b/lib/libmtd.c > @@ -1072,10 +1072,10 @@ int mtd_read(const struct mtd_dev_info *mtd, int fd, int eb, int offs, > mtd->mtd_num, seek); > > while (rd < len) { > - ret = read(fd, buf, len); > + ret = read(fd, buf + rd, len - rd); > if (ret < 0) > return sys_errmsg("cannot read %d bytes from mtd%d (eraseblock %d, offset %d)", > - len, mtd->mtd_num, eb, offs); > + len - rd, mtd->mtd_num, eb, offs + rd); > rd += ret; > } > Patch looks OK. Did you test it? Have you seen MTD drivers that will return short reads? Brian
Hi Brian, Thanks for looking at the patch! On Thu, 2015-11-12 at 11:09 -0800, Brian Norris wrote: > Hi Marcus, > > On Tue, Oct 06, 2015 at 02:13:23PM +0200, Marcus Prebble wrote: > > Hi mtd-list, > ...the current implementation will overwrite the data already > > read in buf which doesn't seem correct. > > In addition, this means we might not actually fill up the entire buffer, > since 2 or more short read()'s might get us to exit the loop with a > cumulative value in 'rd', but only a partially-filled buffer. That could > cause a user to try and handle garbage/uninitialized data. Yes, that is definitely possible and not nice for the user. > Patch looks OK. Did you test it? Have you seen MTD drivers that will > return short reads? I only noticed in passing, not because I was hit by the error. I smoke tested it, but did not hack a driver to return less than len. If I had to guess, I would say that in nearly all cases the drivers do not return short reads otherwise this would probably have been picked up by now. > > Brian
Hi, On Fri, Nov 13, 2015 at 11:07:12AM +0100, Marcus Prebble wrote: > Thanks for looking at the patch! Thanks for the patch! > On Thu, 2015-11-12 at 11:09 -0800, Brian Norris wrote: > > Patch looks OK. Did you test it? Have you seen MTD drivers that will > > return short reads? > > I only noticed in passing, not because I was hit by the error. I smoke > tested it, but did not hack a driver to return less than len. > If I had to guess, I would say that in nearly all cases the drivers do > not return short reads otherwise this would probably have been picked up > by now. OK, that's fine. It's good to know when things are likely to cause real problems vs. when things need fixed just for best practice. BTW, I noticed there isn't a 'Signed-off-by' tag in the patch. It's mostly a formality, but we really shouldn't be taking patches without it. Can you paste one in reply to your patch, and I'll C&P it? Regards, Brian
Hi again and sorry for the delay in replying! On Fri, 2015-11-13 at 11:17 -0800, Brian Norris wrote: > BTW, I noticed there isn't a 'Signed-off-by' tag in the patch. > Can you paste one in reply to your patch, and I'll C&P it? > Signed-off-by: Marcus Prebble <marcus.prebble@axis.com> Kind regards, -Marcus > Regards, > Brian
On Tue, Nov 17, 2015 at 09:43:40AM +0100, Marcus Prebble wrote: > On Fri, 2015-11-13 at 11:17 -0800, Brian Norris wrote: > > > BTW, I noticed there isn't a 'Signed-off-by' tag in the patch. > > Can you paste one in reply to your patch, and I'll C&P it? > > > > Signed-off-by: Marcus Prebble <marcus.prebble@axis.com> Pushed to mtd-utils.git. Thanks!
From 5dccbbd87604665e896472d52f3351c14c24d2b3 Mon Sep 17 00:00:00 2001 From: Marcus Prebble <prebble@axis.com> Date: Mon, 5 Oct 2015 17:32:54 +0200 Subject: [PATCH] mtd_read: Take the buffer offset into account when reading Subsequent calls to read() within the loop will now no longer overwrite the existing contents of buf. --- lib/libmtd.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/libmtd.c b/lib/libmtd.c index 60b4782..bf6d71f 100644 --- a/lib/libmtd.c +++ b/lib/libmtd.c @@ -1072,10 +1072,10 @@ int mtd_read(const struct mtd_dev_info *mtd, int fd, int eb, int offs, mtd->mtd_num, seek); while (rd < len) { - ret = read(fd, buf, len); + ret = read(fd, buf + rd, len - rd); if (ret < 0) return sys_errmsg("cannot read %d bytes from mtd%d (eraseblock %d, offset %d)", - len, mtd->mtd_num, eb, offs); + len - rd, mtd->mtd_num, eb, offs + rd); rd += ret; } -- 2.1.4