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&gt;