Re: [PATCH v3 23/25] tools/xenstore: merge is_valid_nodename() into canonicalize()

[Juergen Gross <jgross@xxxxxxxx>]

On 03.08.23 23:46, Julien Grall wrote:
> Hi,
>
> On 24/07/2023 12:02, Juergen Gross wrote:
> > Today is_valid_nodename() is always called directly after calling
> > canonicalize(), with the exception of do_unwatch(), where the call
> > is missing (which is not correct, but results just in a wrong error
> > reason being returned).
> While this change makes sense...
>
> > Merge is_valid_nodename() into canonicalize(). While at it merge
> > valid_chars() into it, too.
> ... I am not in favor of folding the code is_valid_nodename() and valid_chars() 
> into canonicalize() because the code is now more difficult to read. Also, the 
> keeping the split would allow to free the 'name' in case of an error without 
> adding too much goto in the code.
I don't think we can easily free name in an error case, at that would require
to keep knowledge that name was just allocated in the non-canonical case.

> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V3:
> > - new patch
> > ---
> >   tools/xenstore/xenstored_core.c  | 89 ++++++++++++++------------------
> >   tools/xenstore/xenstored_core.h  |  6 +--
> >   tools/xenstore/xenstored_watch.c | 16 ++----
> >   3 files changed, 45 insertions(+), 66 deletions(-)
> >
> > diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
> > index ea5a1a9cce..ec20bc042d 100644
> > --- a/tools/xenstore/xenstored_core.c
> > +++ b/tools/xenstore/xenstored_core.c
> > @@ -1210,42 +1210,6 @@ void send_ack(struct connection *conn, enum 
> > xsd_sockmsg_type type)
> >       send_reply(conn, type, "OK", sizeof("OK"));
> >   }
> > -static bool valid_chars(const char *node)
> > -{
> > -    /* Nodes can have lots of crap. */
> > -    return (strspn(node,
> > -               "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
> > -               "abcdefghijklmnopqrstuvwxyz"
> > -               "0123456789-/_@") == strlen(node));
> > -}
> > -
> > -bool is_valid_nodename(const struct connection *conn, const char *node,
> > -               bool allow_special)
> > -{
> > -    int local_off = 0;
> > -    unsigned int domid;
> > -
> > -    /* Must start in / or - if special nodes are allowed - in @. */
> > -    if (!strstarts(node, "/") && (!allow_special || !strstarts(node, "@")))
> > -        return false;
> > -
> > -    /* Cannot end in / (unless it's just "/"). */
> > -    if (strends(node, "/") && !streq(node, "/"))
> > -        return false;
> > -
> > -    /* No double //. */
> > -    if (strstr(node, "//"))
> > -        return false;
> > -
> > -    if (sscanf(node, "/local/domain/%5u/%n", &domid, &local_off) != 1)
> > -        local_off = 0;
> > -
> > -    if (domain_max_chk(conn, ACC_PATHLEN, strlen(node) - local_off))
> > -        return false;
> > -
> > -    return valid_chars(node);
> > -}
> > -
> >   /* We expect one arg in the input: return NULL otherwise.
> >    * The payload must contain exactly one nul, at the end.
> >    */
> > @@ -1279,16 +1243,46 @@ static char *perms_to_strings(const void *ctx, const 
> > struct node_perms *perms,
> >   }
> >   const char *canonicalize(struct connection *conn, const void *ctx,
> > -             const char *node)
> > +             const char *node, bool allow_special)
> >   {
> > -    const char *prefix;
> > +    char *name;
> > +    int local_off = 0;
> > +    unsigned int domid;
> > -    if (!node || (node[0] == '/') || (node[0] == '@'))
> > -        return node;
> > -    prefix = get_implicit_path(conn);
> > -    if (prefix)
> > -        return talloc_asprintf(ctx, "%s/%s", prefix, node);
> > -    return node;
> > +    errno = EINVAL;
> > +    if (!node)
> > +        return NULL;
> > +
> > +    if (strspn(node, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
> > +             "0123456789-/_@") != strlen(node))
> > +        return NULL;
> > +
> > +    if (node[0] == '@' && !allow_special)
> > +        return NULL;
> > +
> > +    if (node[0] != '/' && node[0] != '@') {
> > +        name = talloc_asprintf(ctx, "%s/%s", get_implicit_path(conn),
> > +                       node);
> This is allocated but not freed on error. I understand this is part of the 
> 'ctxt' and therefore will be free later on. But this means temporary memory will 
> be allocated for longer. So best to clean-up when you can.
Really?

It is possible, of course, but it is adding more code churn. Remember that
"name" is allocated only in case of a relative path, so freeing it needs
to be conditional, too (yes, it would be possible via comparing name to node).

In case you want me to go this route, I can rearrange the code in order to
avoid multiple error exits by having only one large if () testing for all
possible violations.

> > +        if (!name)
> > +            return NULL;
> > +    } else
> > +        name = (char *)node;
> Why does name need to be const?
I think the question was posed in a wrong way. :-)

I'll change name to be const char *.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature