On Tue, Oct 10, 2023 at 12:00 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> On Tue, Oct 10, 2023 at 12:06 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> >
> > On Fri, 6 Oct 2023 at 19:21, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 6, 2023 at 7:42 PM Ryan Hendrickson
> >
> > > > And there is the escaping that needs to happen for ':' and '\' when
> > > > parsing the path parameters (':' is only special syntax in lowerdir, but
> > > > the escaping logic seems to apply to upperdir and workdir as well, based
> > > > on my testing). Even using the new API, this is handled in the kernel.
> > > > We'd like to know if this escaping can be considered stable as well, and I
> > > > don't think that's a question for the libmount maintainer.
> > >
> > > Agree.
> > > Unlike the comma separated parameters list,
> > > upperdir,workdir,lowerdir are overlayfs specific format.
> > >
> > > ovl_unescape() (for upperdir/workdir) unescapes '\' characters.
> > > as does ovl_parse_param_split_lowerdirs().
> > > Not sure why this was needed for upperdir/workdir, but it It has
> > > been this way for a long time.
> > > I see no reason for it to change in the future.
> >
> > Unescaping upperdir/workdir was the side effect of using a common
> > helper; it wasn't intentional, I think. The problem is that
> > unescaping breaks code that doesn't expect it, and filenames with
> > backslashes (and especially \\ or \: sequences) are very rare, so this
> > won't show up in testing.
> >
> > At this point I'm not sure which is more likely to cause bugs: getting
> > rid of unescaping or leaving it alone.
>
> Considering the fact that the applications that mount overlayfs has
> always had to do the correct escaping, getting rid of escaping can
> only solve issues in new deployments, so I think we should greatly
> favor leaving it alone.
Any change here is a regression. I'm seriously confused why this is
even debated. You already managed to have a regression and I'm still
of the opinion that this should be fixed because it literally breaks
user space.
> >
> > One way out of this mess is to create explicit _unesc versions of these options.
> >
>
> I like that solution, with two reservations:
> 1. IMO, new _unesc versions should only be supported from new mount API
> 2. I only want to do that if real users exists - said users are expected
> to send the patch and explain their use case
This is confusing me a lot. Why would you not want to provide an API
which is clearly, objectively the better API? As user space, when we
can use the new mount API and we could use this, we absolutely would
use this.
> Thanks,
> Amir.
>
