[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] mount: support the sizelimit and offset option for loop devices
From:       Steffen Trumtrar <s.trumtrar () pengutronix ! de>
Date:       2020-10-12 6:37:05
Message-ID: 20201012063705.GD30556 () pengutronix ! de
[Download RAW message or body]

Hi!

On Thu, Oct 01, 2020 at 12:33:19AM +0200, Denys Vlasenko wrote:
> Apologies for an extremely late reply!
> 

No problem!

> 
> On Wed, Jul 29, 2020 at 10:44 AM Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
> > Starting with linux kernel v5.4 squashfs has a more strict parameter
> > checking implemented. Unlike util-linux mount, busybox never supported
> > the sizelimit option but simply forwards it to the kernel.
> > Since v5.4 mounting will fail with
> >
> >     squashfs: Unknown parameter 'sizelimit'
> >
> > Support the sizelimit parameter by setting it in the LOOP_SET_STATUS64
> > structure before handing it to the kernel.
> >
> > While at it also add support for the offset option, which currently will
> > always be set to 0.
> >
> > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> > +                       /* parse options */
> > +                       if (filteropts)
> > +                               for (opt = strtok(filteropts, ","); opt; opt = strtok(NULL, ",")) {
> > +                                       char *opteq = strchr(opt, '=');
> > +                                       int len = strlen(opt);
> > +                                       char *match;
> > +
> > +                                       if (opteq) {
> > +                                               int idx;
> > +                                               static const char options[] ALIGN1 =
> > +                                                       /* 0 */ "offset\0"
> > +                                                       /* 1 */ "sizelimit\0";
> > +
> > +                                               *opteq++ = '\0';
> 
> This will destroy any non-matching options, no?
> (By truncating them at the first '=' - you never restore it)
> 
> > +                                               idx = index_in_strings(options, opt);
> > +                                               switch (idx) {
> > +                                               case 0: // "offset"
> > +                                                       offset = strtoull(opteq, 0, 0);
> 
> No error checking?
> 
> > +                                                       match = strstr(filteropts, opt);
> 
> What are you searching for here? "Where is OPT string in FILTEROPTS?"
> Obviously, it is at OPT pointer - it points into FILTEROPTS string.
> Or worse yet, your FILTEROPTS has a duplicate earlier on, like
> "offset,offset=123"
> and now you will perform this surgery:
> 
> > +                                                       *match = '\0';
> > +                                                       strcat(filteropts, match+len);
> 
> on the *wrong part of the string* (you would delete "offset," not
> "...,offset=123" part).
> Also, you need to use len + 1, no?
> 
> These three lines should be replaced with just
> 
>                          strcpy(opt, opt + len + 1);
> 
> ...but there is another bug: you are operating on strtok'ed string.
> If you edit it like this, subsequent calls of strtok can fail.
> 
> 
> I'm committing a modified patch. Please try current git.
> Thank you.
> 

Thanks for fixing up my messy patch, looks much better.

Thanks,
Steffen

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]