Subject: Re: bug#22460: 'y' command doesn't allow a comment after it one the same line

From: Assaf Gordon <[email-masked]>
Date: Fri, 27 Jan 2017 03:04:27 +0000

Hello Thorsten,

Sorry for the delayed response.

On Mon, Jan 25, 2016 at 02:28:22AM +0100, Thorsten Heymann wrote:
>Unlike other commands, the current implementation of the 'y' command does
>not allow a comment on the same line if the command is not
>"terminated" by a ';'

That is indeed a bug, and a rather old one.

Your suggested fix is correct, but I thought that instead of
duplicating the same few lines, it might be better to extract
the code to a new function.

The attached patch does that and adds a corresponding unit test.

Comments welcomed,
 - assaf

>From 4f6e75fab9999a45205266b3b7620f820ae60e43 Mon Sep 17 00:00:00 2001
From: Assaf Gordon <[email-masked]>
Date: Fri, 27 Jan 2017 02:57:32 +0000
Subject: [PATCH] sed: allow comments,braces after y///

sed-4.3 and earlier rejected the following:
  $ sed 'y/1/a/ #f'
  sed: -e expression #1, char 8: extra characters after command

Reported by Thorsten Heymann in https://bugs.gnu.org/22460 .

* sed/compile.c (read_end_of_cmd): New function, consumes sed script
input until newline, EOF, semicolon, closing brace or comment is found.
(compile_program): Call new function instead of duplicating code.
* testsuite/command-endings.sh: New test, including y/// bug.
* testsuite/local.mk: Add new test.
* NEWS: Mention it.
---
 NEWS                         |   5 ++
 sed/compile.c                |  43 ++++++++------
 testsuite/command-endings.sh | 137 +++++++++++++++++++++++++++++++++++++++++++
 testsuite/local.mk           |   1 +
 4 files changed, 167 insertions(+), 19 deletions(-)
 create mode 100644 testsuite/command-endings.sh

diff --git a/NEWS b/NEWS
index 473965d..93757a5 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,11 @@ GNU sed NEWS                                    -*- outline -*-
 
 * Noteworthy changes in release ?.? (????-??-??) [?]
 
+** Bug fixes
+
+  sed no longer rejects comments and closing braces after y/// commands.
+  [Bug existed at least since sed-3.02]
+
 
 * Noteworthy changes in release 4.3 (2016-12-30) [stable]
 
diff --git a/sed/compile.c b/sed/compile.c
index 966752b..64ddfa4 100644
--- a/sed/compile.c
+++ b/sed/compile.c
@@ -274,6 +274,21 @@ in_nonblank(void)
   return ch;
 }
 
+/* Consume script input until a valid end of command marker is found:
+     comment, closing brace, newline, semicolon or EOF.
+   If any other character is found, die with 'extra characters after command'
+   error.
+*/
+static void
+read_end_of_cmd (void)
+{
+  const int ch = in_nonblank();
+  if (ch == CLOSE_BRACE || ch == '#')
+    savchar(ch);
+  else if (ch != EOF && ch != '\n' && ch != ';')
+    bad_prog(_(EXCESS_JUNK));
+}
+
 /* Read an integer value from the program.  */
 static countT
 in_integer (int ch)
@@ -1103,11 +1118,8 @@ compile_program(struct vector *vector)
             bad_prog(_(EXCESS_CLOSE_BRACE));
           if (cur_cmd->a1)
             bad_prog(_(NO_CLOSE_BRACE_ADDR));
-          ch = in_nonblank();
-          if (ch == CLOSE_BRACE || ch == '#')
-            savchar(ch);
-          else if (ch != EOF && ch != '\n' && ch != ';')
-            bad_prog(_(EXCESS_JUNK));
+
+          read_end_of_cmd ();
 
           vector->v[blocks->v_index].x.jump_index = vector->v_length;
           blocks = release_label(blocks);	/* done with this entry */
@@ -1177,16 +1189,14 @@ compile_program(struct vector *vector)
           if (ISDIGIT(ch) && posixicity != POSIXLY_BASIC)
             {
               cur_cmd->x.int_arg = in_integer(ch);
-              ch = in_nonblank();
             }
           else
-            cur_cmd->x.int_arg = -1;
-
-          if (ch == CLOSE_BRACE || ch == '#')
-            savchar(ch);
-          else if (ch != EOF && ch != '\n' && ch != ';')
-            bad_prog(_(EXCESS_JUNK));
+            {
+              cur_cmd->x.int_arg = -1;
+              savchar (ch);
+            }
 
+          read_end_of_cmd ();
           break;
 
        case '=':
@@ -1203,11 +1213,7 @@ compile_program(struct vector *vector)
         case 'P':
         case 'z':
         case 'x':
-          ch = in_nonblank();
-          if (ch == CLOSE_BRACE || ch == '#')
-            savchar(ch);
-          else if (ch != EOF && ch != '\n' && ch != ';')
-            bad_prog(_(EXCESS_JUNK));
+          read_end_of_cmd ();
           break;
 
         case 'r':
@@ -1348,8 +1354,7 @@ compile_program(struct vector *vector)
                 cur_cmd->x.translate = translate;
               }
 
-            if ((ch = in_nonblank()) != EOF && ch != '\n' && ch != ';')
-              bad_prog(_(EXCESS_JUNK));
+            read_end_of_cmd ();
 
             free_buffer(b);
             free_buffer(b2);
diff --git a/testsuite/command-endings.sh b/testsuite/command-endings.sh
new file mode 100644
index 0000000..41e349c
--- /dev/null
+++ b/testsuite/command-endings.sh
@@ -0,0 +1,137 @@
+#!/bin/sh
+# Test command separators and endings
+
+# Copyright (C) 2017 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=.}/testsuite/init.sh"; path_prepend_ ./sed
+print_ver_ sed
+
+# Allowed endings/separators after most commands:
+#   newline, comment, closing brace, semicolon, EOF
+# they are also allowed after opening and closing braces themselves.
+#
+# Not tested here:
+#   r/R/w/R/e and s///[we] which use read_filename() and do not
+#   accept comments or semicolons.
+
+
+
+# Test commands and braces followed by:
+# closing braces, comment, semicolons, EOF (newlines are tested later).
+#
+# sed-4.3 wrongly rejected y/// followed by '}' or '#' (bug#22460).
+#
+# Implementation notes (see compile.c):
+#   Simple commands, '}', and 'y///' commands use read_end_of_cmd().
+#
+#   q/Q/l/L have additional check for optional integer,
+#   then call read_end_of_cmd().
+#
+#   labels use 'read_label()'.
+#
+#   's///' has special handling, depending on additional flags
+#    (with 's///[we]' commands and semicolons are not allowed).
+#   Implemented in mark_subst_opts().
+#
+for p in \
+    'h'         \
+    'h;'        \
+    'h ;'       \
+    'h# foo'    \
+    'h # foo'   \
+    '{h}'       \
+    '{h } '     \
+    '{ h } '    \
+    \
+    '{h}# foo'  \
+    '{h} # foo' \
+    '{h};'      \
+    '{h} ;'     \
+    '{;h;} '    \
+    '{{h}}'     \
+    '{;{h};}'   \
+    \
+    'y/1/a/'    \
+    'y/1/a/;d'  \
+    'y/1/a/ ;d' \
+    '{y/1/a/}'  \
+    'y/1/a/#foo'\
+    'y/1/a/ #fo'\
+    \
+    's/1/a/'    \
+    's/1/a/;d'  \
+    's/1/a/ ;d' \
+    '{s/1/a/}'  \
+    's/1/a/#foo'\
+    's/1/a/ #fo'\
+    \
+    's/1/a/i ;' \
+    's/1/a/i #foo' \
+    '{ s/1/a/i }' \
+    \
+    'bx; :x'      \
+    'bx; :x;'     \
+    'bx; :x ;'    \
+    'bx; :x#foo'  \
+    'bx; :x #foo' \
+    '{ bx; :x }'  \
+    \
+    'l'           \
+    'l;'          \
+    'l ;'         \
+    'l#foo'       \
+    'l #foo'      \
+    '{l}'         \
+    '{l }'        \
+    'l1'          \
+    'l1;'         \
+    'l1 ;'        \
+    'l1#foo'      \
+    'l1 #foo'     \
+    '{l1}'        \
+    '{l1 }'       \
+    ;
+do
+  sed -n "$p" < /dev/null >out 2>err || fail=1
+  compare /dev/null err || fail=1
+  compare /dev/null out || fail=1
+done
+
+
+# Create files to test newlines after commands
+# (instead of having to embed newlines in shell variables in a portable way)
+printf 'd\n'         > nl1 || framework_failure_
+printf '{\nd}'       > nl2 || framework_failure_
+printf '{d\n}'       > nl3 || framework_failure_
+printf '{d}\n'       > nl4 || framework_failure_
+printf 'y/1/a/\n'    > nl5 || framework_failure_
+printf 's/1/a/\n'    > nl6 || framework_failure_
+printf 'bx\n:x\n'    > nl7 || framework_failure_
+printf 'l\n'         > nl8 || framework_failure_
+printf 'l1\n'        > nl9 || framework_failure_
+# s/// has special allowance for \r in mark_subst_opts(),
+# even if not on windows.
+# TODO: should other commands allow it ?
+printf 's/1/a/\r\n'  > nl10 || framework_failure_
+
+for i in 1 2 3 4 5 6 7 8 9 10 ;
+do
+  sed -n -f "nl$i" </dev/null >out 2>err || fail=1
+  compare /dev/null err || fail=1
+  compare /dev/null out || fail=1
+done
+
+
+Exit $fail
diff --git a/testsuite/local.mk b/testsuite/local.mk
index 2c8c718..732c86d 100644
--- a/testsuite/local.mk
+++ b/testsuite/local.mk
@@ -29,6 +29,7 @@ T =					\
   testsuite/compile-errors.sh		\
   testsuite/compile-tests.sh		\
   testsuite/convert-number.sh		\
+  testsuite/command-endings.sh		\
   testsuite/execute-tests.sh		\
   testsuite/help-version.sh		\
   testsuite/in-place-hyphen.sh		\
-- 
1.9.0