[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 23/25] tools/xenstore: merge is_valid_nodename() into canonicalize()
Hi Juergen,
On 04/08/2023 11:17, Juergen Gross wrote:
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.
Which is pretty much a drop in the ocean in the context of Xenstored :).
In reality most of the validation can be done before the allocation with
a bit of work.
Additionally your example won't validate an absolute pathname at all.
That's an error in the logic. This can be sorted out.
What about this variant:
I still don't see the value of folding is_valid_node_name(). But if the
other agrees with you then...
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) ||
... I would rather keep calling valid_chars() here. The rest looks fine
even though this is definitely not my preference.
(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;
}
Cheers,
--
Julien Grall
|
