Re: [Libguestfs] [PATCH nbdkit v3 2/6] main: Tighten up characters permitted in config keys.
On 9/10/18 10:27 AM, Richard W.M. Jones wrote:
Previously key=value on the command line allowed the key to be
pretty
much anything that didn't contain an '=' character. Even empty
strings were permitted.
This tightens up the permitted keys so they must contain only ASCII
alphanumeric, period, underscore or dash characters; must not be an
empty string; and must start with an ASCII alphabetic character.
---
docs/nbdkit-plugin.pod | 24 +++++++++++++++---------
src/main.c | 36 +++++++++++++++++++++++++++++++++++-
2 files changed, 50 insertions(+), 10 deletions(-)
+Both C<key> and C<value> parameters will be non-NULL.
+
+They key will be a non-empty string beginning with an ASCII alphabetic
s/They/The/
+character (C<A-Z> C<a-z>). The rest of the key must
contain only
+ASCII alphanumeric plus period, underscore or dash characters (C<A-Z>
+C<a-z> C<0-9> C<.> C<_> C<->).
No leading underscore? I'm okay with that, but it is slightly stricter
than C and Shell variables (then again, accepting '.' and '-' already
makes us different). And we can always add stuff later if it proves
useful (it's easier to start strict and relax later, compared to
starting relaxed and then tightening down when we realize the problems
it caused).
+/* When parsing plugin and filter config key=value from the command
+ * line, check that the key is a simple alphanumeric with period,
+ * underscore or dash.
+ *
+ * Note this doesn't return an error. If the key is not valid then we
+ * return false and the parsing code will assume that this is a bare
+ * value instead.
+ */
+static int
+is_config_key (const char *key, size_t len)
+{
+ static const char allowed_first[] =
+ "abcdefghijklmnopqrstuvwxyz"
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
+ static const char allowed[] =
+ "abcdefghijklmnopqrstuvwxyz"
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZ"
+ "0123456789"
+ "._-";
Crazy me is thinking of the micro-optimization:
static const char allowed[] =
"0123456789"
".-_"
"abc..z"
"ABC..Z";
...
+
+ if (len == 0)
+ return 0;
+
+ if (strchr (allowed_first, key[0]) == NULL)
+ return 0;
where this is: 'strchr(allowed + 13, key[0])' (or allowed + 12, if you
allow leading underscore). But please don't - while it's clever, I
don't think it's friendly to future readers, and the optimization is not
in hot code, nor is shaving 52 bytes of .data by avoiding repetition
between the two 'allowed' constants really a deal-breaker.
+
+ /* This works in context of the caller since key[len] == '='. */
+ if (strspn (key, allowed) != len)
+ return 0;
I like it. With the one typo fix,
Reviewed-by: Eric Blake <eblake(a)redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org