Re: [PATCH 5/7] fs: Calculate new position before validtiy check in lseek()

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Wed, Jan 23, 2019 at 11:52 PM Sascha Hauer <s.hauer@pengutronix.de> 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@gmail.com>
> > ---
> >  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@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox