On Wed, Jan 23, 2019 at 11:52 PM Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
>
> On Tue, Jan 22, 2019 at 05:13:36PM -0800, Andrey Smirnov wrote:
> > Calculate new position before validtiy check in lseek() to simplify
> > code a bit as well as make following commit simpler. This should be
> > harmless thing to do, since we don't actually use calculated value
> > unless it passes the validity check.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > ---
> > fs/fs.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/fs.c b/fs/fs.c
> > index 6a62fb98b..9372b9981 100644
> > --- a/fs/fs.c
> > +++ b/fs/fs.c
> > @@ -421,21 +421,21 @@ loff_t lseek(int fildes, loff_t offset, int whence)
> >
> > switch (whence) {
> > case SEEK_SET:
> > + pos = offset;
> > if (f->size != FILE_SIZE_STREAM && offset > f->size)
> > goto out;
> > if (IS_ERR_VALUE(offset))
> > goto out;
>
> This test looks quite unnecessary. Can we remove it? git blame points to
> me of course, but I can't make any sense of it.
>
It originally was "if (offset < 0)", which I think is
reasonable/necessary given how we probably don't want to allow
specifying negative "offset" with SEEK_SET, but allow it for SEEK_END
and SEEK_CUR. Then in attempt to fix /dev/mem accesses on 64-bit MIPS
original check was changed to IS_ERR_VALUE(offset), which made the
original problem less visible (by narrowing down offsets that are
forbidden) which broke lseek() on 32-bit platforms. Hope this
clarifies things a bit.
> > - pos = offset;
> > break;
> > case SEEK_CUR:
> > - if (f->size != FILE_SIZE_STREAM && offset + f->pos > f->size)
> > - goto out;
> > pos = f->pos + offset;
> > + if (f->size != FILE_SIZE_STREAM && pos > f->size)
> > + goto out;
> > break;
> > case SEEK_END:
> > + pos = f->size + offset;
> > if (offset > 0)
> > goto out;
> > - pos = f->size + offset;
>
> When starting to shift the validity checks around, can't we just do them
> after the switch/case instead of in each branch?
>
I think we can move "if (f->size != FILE_SIZE_STREAM && pos >
f->size)" outside. Not sure about some "whence" specific checks. I'll
give it a try in v2.
Thanks,
Andrey Smirnov
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
