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 <br...@clisp.org>
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. */
