bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: quotearg test failures


From: Bruno Haible
Subject: Re: quotearg test failures
Date: Tue, 18 Oct 2016 14:15:28 +0200
User-agent: KMail/4.8.5 (Linux/3.8.0-44-generic; KDE/4.8.5; x86_64; ; )

Hi Pádraig,

> > I disagree with the removal of the '%' checks in tests/test-sh-quote.c
> 
> Agreed. There is a disparity now because there is a switch
> from a longer (shell) to a more concise (c) quoting style
> in certain cases (when just a single quote is encountered).

And what happens more precisely (I've single-stepped it) is:

1) Call to
   quotearg_buffer_restyled (buffer=0x7fffffffd620 "", 
buffersize=18446744073709551615,
                             arg=0x40433f "'foo'bar", argsize=8,
                             quoting_style=shell_quoting_style, flags=0,
                             quote_these_too=0x607018, left_quote=0x0, 
right_quote=0x0)
   at quotearg.c:252

   In line 372 it sets
     quoting_style = shell_always_quoting_style;

   It parses the first character and from line 546 jumps to 
force_outer_quoting_style.

2) Recursive call to
   quotearg_buffer_restyled (buffer=0x7fffffffd620 "", 
buffersize=18446744073709551615,
                             arg=0x40433f "'foo'bar", argsize=8,
                             quoting_style=shell_always_quoting_style, flags=0,
                             quote_these_too=0x0, left_quote=0x0, 
right_quote=0x0)
   at quotearg.c:252

   It processes the entire string and produces
     "''\\''foo'\\''bar"
   which is a string of length 15+NUL.

   Then, in line 715, it finds the following values:
(gdb) print quoting_style
$17 = shell_always_quoting_style
(gdb) print elide_outer_quotes
$18 = false
(gdb) print all_c_and_shell_quote_compat
$19 = true
(gdb) print encountered_single_quote
$20 = true

3) Recursive call to
   quotearg_buffer_restyled (buffer=0x7fffffffd620 "", 
buffersize=18446744073709551615,
                             arg=0x40433f "'foo'bar", argsize=8,
                             quoting_style=c_quoting_style, flags=1,
                             quote_these_too=0x0, left_quote=0x0, 
right_quote=0x0)
   at quotearg.c:252
   and this one produces
     "\"'foo'bar\\'"
   which is a string of length 11+NUL.

The problem is that this code writes 15+1 bytes into the buffer,
then restarts and then reports that it has written 11+1 bytes into
the buffer. shell_quote then knows that it has to allocate 11+1 bytes,
and the second call to quotearg_buffer makes a buffer overrun.

> I'll fix this up either by unsetting a new USE_MOST_CONCISE option
> from shell_quote_{length,copy}

Ugh. This will add complexity to ugliness.

How about this? You are doing multiple passes through the argument string
anyway now. (Originally quotearg was meant as a single-pass implementation,
but it isn't any more.) So
  - remove the code block of lines 711..720,
  - instead, before the for(...) loop, determine all_c_and_shell_quote_compat
    and encountered_single_quote in an extra single pass through the string,
  - and put the code block from lines 711..720 *BEFORE* the for(...) loop.
      
This way, the buffer will only be filled once. => No buffer overrun.

I'm reverting the unintented parts of test-sh-quote.c. You can then decide
how you want to modify quotearg.c (and sh-quote.c if you want).

Bruno


2016-10-18  Bruno Haible  <address@hidden>

        sh-quote, system-quote: revert regression of unit test.
        * tests/test-sh-quote.c (check_one): Do detect buffer overruns.
        * tests/test-system-quote-main.c (check_one): Likewise.

diff --git a/tests/test-sh-quote.c b/tests/test-sh-quote.c
index 091da34..ff05c8e 100644
--- a/tests/test-sh-quote.c
+++ b/tests/test-sh-quote.c
@@ -41,9 +41,11 @@ check_one (const char *input, const char *expected)
 
   ASSERT (output_len <= sizeof (buf) - 2);
   memset (buf, '\0', output_len + 1);
+  buf[output_len + 1] = '%';
   bufend = shell_quote_copy (buf, input);
   ASSERT (bufend == buf + output_len);
   ASSERT (memcmp (buf, output, output_len + 1) == 0);
+  ASSERT (buf[output_len + 1] == '%');
 
   ASSERT (strcmp (output, expected) == 0);
 
diff --git a/tests/test-system-quote-main.c b/tests/test-system-quote-main.c
index 40d7ec6..8eb6a17 100644
--- a/tests/test-system-quote-main.c
+++ b/tests/test-system-quote-main.c
@@ -58,9 +58,11 @@ check_one (enum system_command_interpreter interpreter, 
const char *prog,
 
   ASSERT (output_len <= sizeof (buf) - 2);
   memset (buf, '\0', output_len + 1);
+  buf[output_len + 1] = '%';
   bufend = system_quote_copy (buf, interpreter, input);
   ASSERT (bufend == buf + output_len);
   ASSERT (memcmp (buf, output, output_len + 1) == 0);
+  ASSERT (buf[output_len + 1] == '%');
 
   /* Store INPUT in EXPECTED_DATA_FILE, for verification by the child
      process.  */




reply via email to

[Prev in Thread] Current Thread [Next in Thread]