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

[Juergen Gross <jgross@xxxxxxxx>]

On 04.08.23 11:44, Julien Grall wrote:
> On 04/08/2023 10:34, Juergen Gross wrote:
> > 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).
> I don't mind you want to test against a different value. My point is more that 
> you didn't mention that the limit will be changed.
Will mention it.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature