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

[Juergen Gross <jgross@xxxxxxxx>]

On 04.08.23 12:00, Julien Grall wrote:
> Hi,
>
> On 04/08/2023 10:35, Juergen Gross wrote:
> > 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.
> How about this:
>
> const char *canonicalize(struct connection *conn, const void *ctx,
>                           const char *node, bool allow_special)
> {
>          const char *prefix;
>          const char *name;
>
>          if (!node)
>                  return NULL;
>
>          if (node[0] == '@' && !allow_special)
>                  return NULL;
>
>          if (!node || (node[0] == '/') || (node[0] == '@'))
>                  return node;
>          prefix = get_implicit_path(conn);
>          if (prefix) {
>                  name = talloc_asprintf(ctx, "%s/%s", prefix, node);
>                  if (name)
>                          return NULL;
>          } else
>                  name = node;
>
>          if (!is_valid_nodename(conn, node, allow_special)) {
>                  /* Release the memory if 'name' was allocated by us */
>                  if ( name != node )
>                          talloc_free(name);
>                  return NULL;
>          }
>
>          return name;
> }
>
> And before you ask, I don't see the benefits to partially validate the name 
> before allocating. Hence why I suggest to keep is_valid_nodename() as this keep 
> the function small.
Partially validating before doing the allocation is a benefit as it doesn't
spend cycles on validating the known good prefix.

Additionally your example won't validate an absolute pathname at all.

What about this variant:

const char *canonicalize(struct connection *conn, const void *ctx,
                         const char *node, bool allow_special)
{
        const char *name;
        int local_off = 0;
        unsigned int domid;

        /*
         * Invalid if any of:
         * - no node at all
         * - illegal character in node
         * - starts with '@' but no special node allowed
         */
        errno = EINVAL;
        if (!node ||
            strspn(node, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
                         "0123456789-/_@") != strlen(node) ||
            (node[0] == '@' && !allow_special))
                return NULL;

        if (node[0] != '/' && node[0] != '@') {
                name = talloc_asprintf(ctx, "%s/%s", get_implicit_path(conn),
                                       node);
                if (!name)
                        return NULL;
        } else
                name = node;

        if (sscanf(name, "/local/domain/%5u/%n", &domid, &local_off) != 1)
                local_off = 0;

        /*
         * Only valid if:
         * - doesn't end in / (unless it's just "/")
         * - no double //
         * - not violating max allowed path length
         */
        if (!(strends(name, "/") && !streq(name, "/")) &&
            !strstr(name, "//") &&
            !domain_max_chk(conn, ACC_PATHLEN, strlen(name) - local_off))
                return name;

        /* Release the memory if 'name' was allocated by us. */
        if (name != node)
                talloc_free(name);
        return NULL;
}


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature