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

[Julien Grall <julien@xxxxxxx>]

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.
> 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.
> +               if (!name)
> +                       return NULL;
> +       } else
> +               name = (char *)node;
Why does name need to be const?

> +
> +       /* Cannot end in / (unless it's just "/"). */
> +       if (strends(name, "/") && !streq(name, "/"))
> +               return NULL;
> +
> +       /* No double //. */
> +       if (strstr(name, "//"))
> +               return NULL;
> +
> +       if (sscanf(name, "/local/domain/%5u/%n", &domid, &local_off) != 1)
> +               local_off = 0;
> +
> +       if (domain_max_chk(conn, ACC_PATHLEN, strlen(name) - local_off))
> +               return NULL;
> +
> +       return name;
>   }
Cheers,

--
Julien Grall