Subject: bug#23635: closed (Re: bug#23635: possible bug in \c escape handling)

From: [email-masked] (GNU bug Tracking System)
Date: Mon, 30 May 2016 02:01:02 +0000

Your bug report

#23635: possible bug in \c escape handling

which was filed against the sed package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
23635: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23635
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#23635: possible bug in \c escape handling
From: Jim Meyering <[email-masked]>
To: Assaf Gordon <[email-masked]>
Cc: [email-masked]
Date: Sun, 29 May 2016 19:00:25 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On Sat, May 28, 2016 at 8:40 PM, Jim Meyering <[email-masked]> wrote:
> On Sat, May 28, 2016 at 7:31 PM, Assaf Gordon <[email-masked]> wrote:
>> Hello,
>>
>> Thank you for the review.
>> Attached an improved version.
>>
>> Regarding when the bug was introduced (in the 'NEWS'), version 3.02 did not support \c escapes, and version 4.0.6 had this bug (as does the first git commit). I wrote:
>>     [bug introduced in the sed-4.0.* releases]
>
> Thanks for the quick update!
> One more thing I noticed is that you use here docs
> that interpolate. Just as I prefer to use single-quoted strings
> most of the time, e.g., to avoid having to backslash-escape
> every backslash, I prefer to use quoted here docs as in the
> attached. Also, I prefer to space-delimit operators like '<', '<<', and '>'.
> At least on one side.
>
> Also, I removed a stray "EOF" after the final "Exit..." line.
>
> Here's the proposed delta, on top of your patch:

I've pushed your patch amended with that change, and tweaked the log
message to have these lines: capitalized first word of each sentence
and added the (T) and (Bug fixes) qualifiers:

    * sed/compile.c: (RECURSIVE_ESCAPE_C): New error message.
    (normalize_text): Check for \c-backslash, reject recursive escaping.
    * testsuite/recursive-escape-c.sh: New file. Test new behaviour.
    * testsuite/Makefile.am (T): Add new test.
    * NEWS (Bug fixes): Mention it.

Thanks again.
As I write this, I realized that I should have referenced the bug
report URL in the commit log.  Oh well.

Subject: possible bug in \c escape handling
From: Assaf Gordon <[email-masked]>
To: [email-masked]
Date: Fri, 27 May 2016 21:08:21 -0400
List-Id: <debbugs-submit.debbugs.gnu.org>

Hello,

There might be a small bug in processing of GNU extension escape sequence "\c".

When the character following "\c" is a backslash, the code consumes only one character, leading to inconsistent and incorrect output.
Example:

   $ echo a | sed 's/./\c\\/' | od -c
   0000000 034 \ \n
   0000003
   $ echo a | sed 's/./\c\d/' | od -c
   0000000 034 d \n
   0000003

but:

   $ echo a | sed 's/./\c\/' | od -c
   sed: -e expression #1, char 8: unterminated `s' command
   0000000

Meaning there is no way to generate the character '\x034' alone with "\c".

This is also somewhat inconsistent because it consumes a single backslash character
(whereas everywhere else a single backslash is the escape character itself).

For comparison, other characters behave as expected:

   $ sed 's/./\cA/' in | od -c
   0000000 001 \n
   0000002
   $ sed 's/./\c[/' in | od -c
   0000000 033 \n
   0000002
   $ sed 's/./\c]/' in | od -c
   0000000 035 \n
   0000002

As a side effect, it could also be confusing if the syntax allows 'recursive' escapes,
such as "\c\x41", which might be argued to be '\c' of the following character,
which should be first evaluated as \x61, resulting in "\cA".

The attached patch fixes the problem with the following rules:
1. '\c\\' = Control-Backslash = ASCII 0x34.
2. Any other backslash combinations after "\c" are rejected, and sed aborts.

Tests included. comments are welcomed.

- assaf

>From 4512ebbaf753c32e2515f582382e3b3bb10fa781 Mon Sep 17 00:00:00 2001
From: Assaf Gordon <[email-masked]>
Date: Fri, 27 May 2016 20:49:33 -0400
Subject: [PATCH] sed: reject recursive escaping after \c

previously, sed 's/./\c\\/' or 's/./\c\d/' would produce incorrect
results. Require two backslashes after \c to denote control sequence
^\ (ASCII 0x34), and reject recursive escaping (e.g. \c\x61).

* sed/compile.c: (RECURSIVE_ESCAPE_C): new error message;
(normalize_text): check for \c-backslash, reject recursive escaping.
* testsuite/recursive-escape-c.sh: test new behaviour.
* testsuite/Makefile.am: add new test.
---
 sed/compile.c                   | 11 +++++-
 testsuite/Makefile.am           |  1 +
 testsuite/recursive-escape-c.sh | 83 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+), 1 deletion(-)
 create mode 100644 testsuite/recursive-escape-c.sh

diff --git a/sed/compile.c b/sed/compile.c
index 270caaf..c039c24 100644
--- a/sed/compile.c
+++ b/sed/compile.c
@@ -138,7 +138,8 @@ static const char errors[] =
   "invalid usage of line address 0\0"
   "unknown command: `%c'\0"
   "incomplete command\0"
-  "\":\" lacks a label";
+  "\":\" lacks a label\0"
+  "recursive escaping after \\c not allowed";
 
 #define BAD_BANG (errors)
 #define BAD_COMMA (BAD_BANG + sizeof(N_("multiple `!'s")))
@@ -182,6 +183,8 @@ static const char errors[] =
 #define INCOMPLETE_CMD (UNKNOWN_CMD + sizeof(N_("unknown command: `%c'")))
 #define COLON_LACKS_LABEL (INCOMPLETE_CMD \
   + sizeof(N_("incomplete command")))
+#define RECURSIVE_ESCAPE_C (COLON_LACKS_LABEL \
+  + sizeof(N_("\":\" lacks a label")))
 /* #define END_ERRORS (COLON_LACKS_LABEL + sizeof(N_("\":\" lacks a label"))) */
 
 static struct output *file_read = NULL;
@@ -1466,6 +1469,12 @@ convert:
             if (++p < bufend)
               {
                 *q++ = toupper((unsigned char) *p) ^ 0x40;
+                if (*p == '\\')
+                  {
+                    p++;
+                    if (*p != '\\')
+                      bad_prog(RECURSIVE_ESCAPE_C);
+                  }
                 p++;
                 continue;
               }
diff --git a/testsuite/Makefile.am b/testsuite/Makefile.am
index b37e402..fd4f19e 100644
--- a/testsuite/Makefile.am
+++ b/testsuite/Makefile.am
@@ -12,6 +12,7 @@ T =				\
   in-place-hyphen.sh		\
   invalid-mb-seq-UMR.sh		\
   range-overlap.sh		\
+  recursive-escape-c.sh		\
   subst-mb-incomplete.sh	\
   temp-file-cleanup.sh
 
diff --git a/testsuite/recursive-escape-c.sh b/testsuite/recursive-escape-c.sh
new file mode 100644
index 0000000..4ce46dd
--- /dev/null
+++ b/testsuite/recursive-escape-c.sh
@@ -0,0 +1,83 @@
+#!/bin/sh
+# test \c escaping
+
+# Copyright (C) 2016 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+. "${srcdir=.}/init.sh"; path_prepend_ ../sed
+print_ver_ sed
+
+fail=0
+unset POSIXLY_CORRECT
+export LC_ALL=C
+
+# input file, any 6 lines would do, each a different test case
+cat<<EOF>in1 || framework_failure_
+a
+a
+a
+a
+a
+a
+EOF
+
+# input program
+# (NOTE: every two-backslashes in the here-doc result
+#  in one backslash in the file)
+cat<<EOF>prog1 || framework_failure_
+1s/./\\cA/
+2s/./\\cB/
+3s/./\\c[/
+4s/./\\c]/
+
+# '\c' at end-of-buffer, a backslash is pushed up
+# on level of interpretation, and the '.' match is replaced
+# with one backslash.
+5s/./\\c/
+
+# This would return incorrect results before 4.3,
+# producing both \034 and another backslash.
+6s/./\\c\\\\/
+EOF
+
+# expected output:
+printf '\001\n\002\n\033\n\035\n\\\n\034\n' > exp1 || framework_failure_
+
+#
+# Run simple test cases
+#
+sed -f prog1 in1 > out1 || fail=1
+compare_ exp1 out1 || fail=1
+
+# for easier troubleshooting, if users ever report errors
+if test "$fail" -eq 1 ; then
+    od -tx1c prog1
+    od -tx1c exp1
+    od -tx1c out1
+fi
+
+#
+# Test invalid usage
+#
+cat<<EOF>exp-err || framework_failure_
+sed: -e expression #1, char 10: recursive escaping after \c not allowed
+EOF
+
+# Before sed-4.3, this resulted in '\034d' .
+# now it should be rejected.
+returns_ 1 sed '1s/./\c\d/' in1 2>err || fail=1
+compare_ exp-err err || fail=1
+
+Exit $fail
+EOF
-- 
2.8.2