Re: [PATCH v3 22/25] tools/xenstore: merge get_spec_node() into get_node_canonicalized()

[Juergen Gross <jgross@xxxxxxxx>]

On 04.08.23 11:21, Julien Grall wrote:
> Hi,
>
> On 04/08/2023 10:17, Juergen Gross wrote:
> > On 03.08.23 23:36, Julien Grall wrote:
> > > Hi,
> > >
> > > On 24/07/2023 12:02, Juergen Gross wrote:
> > > > diff --git a/tools/xenstore/xenstored_watch.c 
> > > > b/tools/xenstore/xenstored_watch.c
> > > > index 86cf8322b4..2662a3fa49 100644
> > > > --- a/tools/xenstore/xenstored_watch.c
> > > > +++ b/tools/xenstore/xenstored_watch.c
> > > > @@ -166,19 +166,12 @@ static int destroy_watch(void *_watch)
> > > >   static int check_watch_path(struct connection *conn, const void *ctx,
> > > >                   const char **path, bool *relative)
> > > >   {
> > > > -    /* Check if valid event. */
> > > > -    if (strstarts(*path, "@")) {
> > > > -        *relative = false;
> > > > -        if (strlen(*path) > XENSTORE_REL_PATH_MAX)
> > > > -            goto inval;
> > > I can't find an exact matching check in is_valid_nodename(). The call also 
> > > seems to put more restriction on '@' node. Can you clarify?
> > The call of domain_max_chk() in is_valid_nodename() will check the length
> > of the node name (at least for unprivileged callers, which is the important
> > case).
> Right, but from my understanding, this may not check against 
> XENSTORE_REL_PATH_MAX but whatever limit the user set.
Yes, and that's what should be tested, right? I don't see why special nodes
should not adhere to the same limits as other nodes. In case an unprivileged
user should have access to special nodes, the limits shouldn't hinder the
user to access those nodes (setting a limit below 15 would be ridiculous
anyway, and that is the length of longest special node name today).

> This is a change of behavior that you ought to be explained.
>
> > The additional restrictions for special nodes are:
> >
> > - they can't end with "/"
> > - they can't contain "//"
> > - they can't contain characters other than the ones allowed for normal nodes
> >
> > None of those restrictions are problematic. I can add something to the
> > commit message if you want.
> Yes please.
Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature