[1/3] libxlu_cfg: reject unknown characters following '\'

Commit Message

Jim Fehlig Feb. 17, 2016, 3:54 a.m. UTC

When dequoting config strings in xlu__cfgl_dequote(), unknown
characters following a '\', and the '\' itself, are discarded.
E.g. a disk configuration string containing

  rbd:pool/image:mon_host=192.168.0.100\:6789

would be dequoted as

  rbd:pool/image:mon_host=192.168.0.1006789

Instead of discarding the '\' and unknown character, reject the
string and set error to EINVAL.
---
 tools/libxl/libxlu_cfg.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Ian Campbell Feb. 17, 2016, 10:05 a.m. UTC | #1

On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
> When dequoting config strings in xlu__cfgl_dequote(), unknown
> characters following a '\', and the '\' itself, are discarded.
> E.g. a disk configuration string containing
> 
>   rbd:pool/image:mon_host=192.168.0.100\:6789
> 
> would be dequoted as
> 
>   rbd:pool/image:mon_host=192.168.0.1006789
> 
> Instead of discarding the '\' and unknown character, reject the
> string and set error to EINVAL.

Missing your S-o-b.

Other than that:

> +                xlu__cfgl_lexicalerror(ctx, "invalid character after backlash "
> +                                       "in quoted string");

Please try where possible not to split string constants (so log messages
can more easily be grepped for). If that would result in more than 80
characters then it is acceptable to pull the string in more than you would
normally, e.g if this is (as I expect it will be) too long:

                xlu__cfgl_lexicalerror(ctx,
                                       "invalid character after backlash in quoted string");

then it's ok to do:
                xlu__cfgl_lexicalerror(ctx,
                    "invalid
character after backlash in quoted string");

(I usually try and keep it to a multiple of 4 spaces indent, FWIW) or for really long strings even:

                xlu__cfgl_lexicalerror(ctx,
 "invalid character after backlash in quoted string blah blah blah");

is preferable to breaking the string in half.

Ian.

Ian Campbell Feb. 17, 2016, 10:11 a.m. UTC | #2

On Wed, 2016-02-17 at 10:05 +0000, Ian Campbell wrote:
> On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
> > When dequoting config strings in xlu__cfgl_dequote(), unknown
> > characters following a '\', and the '\' itself, are discarded.
> > E.g. a disk configuration string containing
> > 
> >   rbd:pool/image:mon_host=192.168.0.100\:6789
> > 
> > would be dequoted as
> > 
> >   rbd:pool/image:mon_host=192.168.0.1006789
> > 
> > Instead of discarding the '\' and unknown character, reject the
> > string and set error to EINVAL.
> 
> Missing your S-o-b.
> 
> Other than that:
> 
> > +                xlu__cfgl_lexicalerror(ctx, "invalid character after
> > backlash "
> > +                                       "in quoted string");
> 
> Please try where possible not to split string constants (so log messages
> can more easily be grepped for).

I see now that this parsing code is pretty liberally ignoring this advice
already. So apart from the missing S-o-b this patch is

    Acked-by: Ian Campbell <    [e-mail]    >

Ian.

Jim Fehlig Feb. 17, 2016, 5:26 p.m. UTC | #3

On 02/17/2016 03:11 AM, Ian Campbell wrote:
> On Wed, 2016-02-17 at 10:05 +0000, Ian Campbell wrote:
>> On Tue, 2016-02-16 at 20:54 -0700, Jim Fehlig wrote:
>>> When dequoting config strings in xlu__cfgl_dequote(), unknown
>>> characters following a '\', and the '\' itself, are discarded.
>>> E.g. a disk configuration string containing
>>>
>>>   rbd:pool/image:mon_host=192.168.0.100\:6789
>>>
>>> would be dequoted as
>>>
>>>   rbd:pool/image:mon_host=192.168.0.1006789
>>>
>>> Instead of discarding the '\' and unknown character, reject the
>>> string and set error to EINVAL.
>> Missing your S-o-b.
>>
>> Other than that:
>>
>>> +                xlu__cfgl_lexicalerror(ctx, "invalid character after
>>> backlash "
>>> +                                       "in quoted string");
>> Please try where possible not to split string constants (so log messages
>> can more easily be grepped for).
> I see now that this parsing code is pretty liberally ignoring this advice
> already. So apart from the missing S-o-b this patch is

Your suggestion is a good one and is common throughout much of libxl, so I
pulled back the error string indentation in V2.

Regards,
Jim

Patch

diff --git a/tools/libxl/libxlu_cfg.c b/tools/libxl/libxlu_cfg.c
index 1d70909..f8e0bc7 100644
--- a/tools/libxl/libxlu_cfg.c
+++ b/tools/libxl/libxlu_cfg.c
@@ -533,6 +533,11 @@  char *xlu__cfgl_dequote(CfgParseContext *ctx, const char *src) {
                 NUMERIC_CHAR(2,2,16,"hex");
             } else if (nc>='0' && nc<='7') {
                 NUMERIC_CHAR(1,3,10,"octal");
+            } else {
+                xlu__cfgl_lexicalerror(ctx, "invalid character after backlash "
+                                       "in quoted string");
+                ctx->err= EINVAL;
+                goto x;
             }
             assert(p <= src+len-1);
         } else {