sed: Fix read/write file not optional

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

sed: Fix read/write file not optional

Martijn van Duren-5
Hello tech@,

Based on the diff from kshe I found a small inconsistency in our
implementation.
Right now we allow r to have no argument, where posix states[0]:
The r and w command verbs, and the w flag to the s command, take an
rfile (or wfile) parameter, separated from the command verb letter or
flag by one or more <blank> characters; implementations may allow zero  
separation as an extension.

Our behaviour saves an empty string in the command, which results in a
failed fopen, which gets silently ignored.

FreeBSD does enforces the rfile since 1997 and NetBSD since 2014.
gsed doesn't do any check and behaves the same as our r command. gsed's
w command just complains that it can't open the empty filename, so the  
only check there is on the (f)open part.

The patch below enforces the rfile, removes HISTORIC_PRACTICE from
the w-flag (which isn't enabled, since forever), makes the w-flag use
the same-code path as w command (which already is what does, so it just
simplifies the code) and changes the message everywhere to
"filename expected", instead of "no {r,w}file specified". Since a ';'
doesn't delimit the command/flag it should more than suffice here and
makes the messages more uniform.

OK?

martijn@

[0]http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

Index: compile.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/compile.c,v
retrieving revision 1.42
diff -u -r1.42 compile.c
--- compile.c 1 Aug 2017 18:05:53 -0000 1.42
+++ compile.c 7 Dec 2017 10:01:08 -0000
@@ -277,6 +277,8 @@
  pledge_rpath = 1;
  p++;
  EATSPACE();
+ if (*p == '\0')
+ error(COMPILE, "filename expected");
  cmd->t = duptoeol(p, "read command", NULL);
  break;
  case BRANCH: /* b t */
@@ -539,7 +541,6 @@
 {
  int gn; /* True if we have seen g or n */
  long l;
- char wfile[PATH_MAX], *q, *eq;
 
  s->n = 1; /* Default */
  s->p = 0;
@@ -577,32 +578,16 @@
  continue;
  case 'w':
  p++;
-#ifdef HISTORIC_PRACTICE
- if (*p != ' ') {
- warning("space missing before w wfile");
- return (p);
- }
-#endif
  EATSPACE();
- q = wfile;
- eq = wfile + sizeof(wfile) - 1;
- while (*p) {
- if (*p == '\n')
- break;
- if (q >= eq)
- error(COMPILE, "wfile too long");
- *q++ = *p++;
- }
- *q = '\0';
- if (q == wfile)
- error(COMPILE, "no wfile specified");
- s->wfile = strdup(wfile);
+ if (*p == '\0')
+ error(COMPILE, "filename expected");
+ s->wfile = duptoeol(p, "s command w flag", NULL);
  if (aflag)
  pledge_wpath = 1;
- else if ((s->wfd = open(wfile,
+ else if ((s->wfd = open(s->wfile,
     O_WRONLY|O_APPEND|O_CREAT|O_TRUNC,
     DEFFILEMODE)) == -1)
- error(FATAL, "%s: %s", wfile, strerror(errno));
+ error(FATAL, "%s: %s", s->wfile, strerror(errno));
  return (p);
  default:
  error(COMPILE,
Index: sed.1
===================================================================
RCS file: /cvs/src/usr.bin/sed/sed.1,v
retrieving revision 1.50
diff -u -r1.50 sed.1
--- sed.1 19 Jul 2017 21:28:19 -0000 1.50
+++ sed.1 7 Dec 2017 10:01:08 -0000
@@ -255,7 +255,7 @@
 flag to the
 .Ic s
 function,
-take an optional
+take a
 .Ar file
 parameter,
 which should be separated from the function or flag by whitespace.

Reply | Threaded
Open this post in threaded view
|

Re: sed: Fix read/write file not optional

Todd C. Miller-2
Previously, 'p' would be traversed until eol.  With your diff this
is not longer the case.  Since the return value of compile_flags()
is used to assign a new value for 'p', isn't that a problem?

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: sed: Fix read/write file not optional

Martijn van Duren-5
On 12/07/17 21:25, Todd C. Miller wrote:
> Previously, 'p' would be traversed until eol.  With your diff this
> is not longer the case.  Since the return value of compile_flags()
> is used to assign a new value for 'p', isn't that a problem?
>
>  - todd
>
Since the w flag works to eol everything usable has been consumed. The
only use for p after compile_flags is for a semicolon check. So the only
way the position of p could have effect is when the filename starts with
"[[:space:]]*;". Since no sane person would start a filename with a
semicolon it would cause a compiletime error and stop the run, so we
could call it a feature. /joke

But you're right, although I would call it a bug more than a problem,
since this scenario would most likely never occur. Below is a revised
diff. This invalidates p, by turning it into a NUL. This is safe since
duptoeol also writes to the same string.

martijn@

Index: compile.c
===================================================================
RCS file: /cvs/src/usr.bin/sed/compile.c,v
retrieving revision 1.42
diff -u -p -u -r1.42 compile.c
--- compile.c 1 Aug 2017 18:05:53 -0000 1.42
+++ compile.c 8 Dec 2017 06:24:42 -0000
@@ -277,6 +277,8 @@ nonsel: /* Now parse the command */
  pledge_rpath = 1;
  p++;
  EATSPACE();
+ if (*p == '\0')
+ error(COMPILE, "filename expected");
  cmd->t = duptoeol(p, "read command", NULL);
  break;
  case BRANCH: /* b t */
@@ -539,7 +541,6 @@ compile_flags(char *p, struct s_subst *s
 {
  int gn; /* True if we have seen g or n */
  long l;
- char wfile[PATH_MAX], *q, *eq;
 
  s->n = 1; /* Default */
  s->p = 0;
@@ -577,32 +578,17 @@ compile_flags(char *p, struct s_subst *s
  continue;
  case 'w':
  p++;
-#ifdef HISTORIC_PRACTICE
- if (*p != ' ') {
- warning("space missing before w wfile");
- return (p);
- }
-#endif
  EATSPACE();
- q = wfile;
- eq = wfile + sizeof(wfile) - 1;
- while (*p) {
- if (*p == '\n')
- break;
- if (q >= eq)
- error(COMPILE, "wfile too long");
- *q++ = *p++;
- }
- *q = '\0';
- if (q == wfile)
- error(COMPILE, "no wfile specified");
- s->wfile = strdup(wfile);
+ if (*p == '\0')
+ error(COMPILE, "filename expected");
+ s->wfile = duptoeol(p, "s command w flag", NULL);
+ *p = '\0';
  if (aflag)
  pledge_wpath = 1;
- else if ((s->wfd = open(wfile,
+ else if ((s->wfd = open(s->wfile,
     O_WRONLY|O_APPEND|O_CREAT|O_TRUNC,
     DEFFILEMODE)) == -1)
- error(FATAL, "%s: %s", wfile, strerror(errno));
+ error(FATAL, "%s: %s", s->wfile, strerror(errno));
  return (p);
  default:
  error(COMPILE,
Index: sed.1
===================================================================
RCS file: /cvs/src/usr.bin/sed/sed.1,v
retrieving revision 1.51
diff -u -p -u -r1.51 sed.1
--- sed.1 7 Dec 2017 09:52:26 -0000 1.51
+++ sed.1 8 Dec 2017 06:24:42 -0000
@@ -255,7 +255,7 @@ as well as the
 flag to the
 .Ic s
 function,
-take an optional
+take a
 .Ar file
 parameter,
 which should be separated from the function or flag by whitespace.

Reply | Threaded
Open this post in threaded view
|

Re: sed: Fix read/write file not optional

Todd C. Miller-2
On Fri, 08 Dec 2017 07:31:25 +0100, Martijn van Duren wrote:

> But you're right, although I would call it a bug more than a problem,
> since this scenario would most likely never occur. Below is a revised
> diff. This invalidates p, by turning it into a NUL. This is safe since
> duptoeol also writes to the same string.

I was considering the same solution so OK millert@

 - todd