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

[Julien Grall <julien@xxxxxxx>]

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.
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.

Cheers,

--
Julien Grall