'expr' problems with strcoll and POSIX (coreutils 5.0.1)

[Paul Eggert]

The letter from Andreas about 'uniq' prompted me to look for other
instances of 'strcoll' in coreutils.  I found two problematic
instances, both in expr.c.  One instance should be strcmp; the other
instance should report an error if strcoll fails.  While looking at
expr.c I noticed another problem with POSIX conformance: it doesn't
distinguish between exit status 2 and 3 as POSIX 1003.1-2001 requires.

Here is a proposed patch for both problems.

diff -pru coreutils/ChangeLog coreutils-strcoll/ChangeLog
--- coreutils/ChangeLog Mon Jul 14 23:55:22 2003
+++ coreutils-strcoll/ChangeLog Thu Jul 17 14:47:27 2003
@@ -1,3 +1,22 @@
+2003-07-17  Paul Eggert  <address@hidden>
+
+       * src/expr.c: Include "exitfail.h", "quotearg.h".
+       (EXPR_INVALID, EXPR_ERROR): New constants.
+       (nomoreargs, null, toarith, nextarg): Return bool, not int.
+       (syntax_error): New function, exiting with status 2.  Use it
+       insteading of printing "syntax error" ourselves.
+       (main): Initialize exit_failure to EXPR_ERROR.
+       Exit with EXPR_INVALID on syntax error (too few arguments).
+       (nextarg): Use strcmp, not strcoll; strcoll might return
+       an undesirable 0, or might fail.
+       (docolon, eval4, eval3): Exit with status 3 on invalid argument type
+       or other such error.
+       (eval2): Report an error if strcoll fails in a string comparison.
+       * src/sort.c: Include "exitfail.h".
+       (main): Set exit_failure, not xalloc_exit_failure and
+       xmemcoll_exit_failure.
+       * tests/expr/basic: Invalid value exits with status 3, not 2.
+       
 2003-07-15  Jim Meyering  <address@hidden>
 
        * Version 5.0.1.
diff -pru coreutils/NEWS coreutils-strcoll/NEWS
--- coreutils/NEWS      Sun Jul 13 23:30:32 2003
+++ coreutils-strcoll/NEWS      Thu Jul 17 14:53:02 2003
@@ -1,4 +1,9 @@
 GNU coreutils NEWS                                    -*- outline -*-
+
+* expr now exits with status 2 if the expression is syntactically valid, and
+  with status 3 if an error occurred.  POSIX requires this.
+* expr now reports trouble if string comparison fails due to a collation error.
+
 * Major changes in release 5.0.1:
 
 ** New programs
diff -pru coreutils/doc/ChangeLog coreutils-strcoll/doc/ChangeLog
--- coreutils/doc/ChangeLog     Mon Jul 14 23:39:54 2003
+++ coreutils-strcoll/doc/ChangeLog     Thu Jul 17 14:41:10 2003
@@ -1,3 +1,9 @@
+2003-07-17  Paul Eggert  <address@hidden>
+
+       * coreutils.texi (expr invocation): Exit status is 2 if the
+       expression is syntactically invalid, 3 if there is some other error.
+       This change is for conformance to POSIX.
+
 2003-07-14  Paul Eggert  <address@hidden>
 
        * doc/coreutils.texi (uname invocation): Explain the POSIX
diff -pru coreutils/doc/coreutils.texi coreutils-strcoll/doc/coreutils.texi
--- coreutils/doc/coreutils.texi        Mon Jul 14 23:39:49 2003
+++ coreutils-strcoll/doc/coreutils.texi        Thu Jul 17 14:20:27 2003
@@ -8996,7 +8996,8 @@ Exit status:
 @display
 0 if the expression is neither null nor 0,
 1 if the expression is null or 0,
-2 for invalid expressions.
+2 if the expression is syntactically invalid,
+3 if an error occurred.
 @end display
 
 @menu
diff -pru coreutils/lib/ChangeLog coreutils-strcoll/lib/ChangeLog
--- coreutils/lib/ChangeLog     Mon Jul 14 11:58:28 2003
+++ coreutils-strcoll/lib/ChangeLog     Thu Jul 17 14:39:28 2003
@@ -1,3 +1,9 @@
+2003-07-17  Paul Eggert  <address@hidden>
+
+       * xalloca.h, xmalloc.c, xmemcoll.c, xmemcoll.h:
+       Merge with gnulib.  Use a single exit_failure variable rather
+       than a separate one for each module.
+
 2003-07-14  Jim Meyering  <address@hidden>
 
        * save-cwd.h: Add copyright.
diff -pru coreutils/lib/xalloc.h coreutils-strcoll/lib/xalloc.h
--- coreutils/lib/xalloc.h      Wed Jun 18 01:10:03 2003
+++ coreutils-strcoll/lib/xalloc.h      Thu Jul 17 11:30:11 2003
@@ -1,6 +1,7 @@
 /* xalloc.h -- malloc with out-of-memory checking
 
-   Copyright (C) 1990-2000, 2003 Free Software Foundation, Inc.
+   Copyright (C) 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998,
+   1999, 2000, 2003 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
@@ -31,11 +32,6 @@
 #  define ATTRIBUTE_NORETURN __attribute__ ((__noreturn__))
 # endif
 
-/* Exit value when the requested amount of memory is not available.
-   It is initialized to EXIT_FAILURE, but the caller may set it to
-   some other value.  */
-extern int xalloc_exit_failure;
-
 /* If this pointer is non-zero, run the specified function upon each
    allocation failure.  It is initialized to zero. */
 extern void (*xalloc_fail_func) (void);
@@ -46,7 +42,8 @@ extern void (*xalloc_fail_func) (void);
 extern char const xalloc_msg_memory_exhausted[];
 
 /* This function is always triggered when memory is exhausted.  It is
-   in charge of honoring the three previous items.  This is the
+   in charge of honoring the two previous items.  It exits with status
+   exit_failure (defined in exitfail.h).  This is the
    function to call when one wants the program to die because of a
    memory allocation failure.  */
 extern void xalloc_die (void) ATTRIBUTE_NORETURN;
@@ -56,10 +53,10 @@ void *xcalloc (size_t n, size_t s);
 void *xrealloc (void *p, size_t n);
 char *xstrdup (const char *str);
 
-# define XMALLOC(Type, N_items) xmalloc (sizeof (Type) * (N_items))
-# define XCALLOC(Type, N_items) xcalloc (sizeof (Type), (N_items))
-# define XREALLOC(Ptr, Type, N_items) xrealloc ((Ptr), \
-                                               sizeof (Type) * (N_items))
+# define XMALLOC(Type, N_items) ((Type *) xmalloc (sizeof (Type) * (N_items)))
+# define XCALLOC(Type, N_items) ((Type *) xcalloc (sizeof (Type), (N_items)))
+# define XREALLOC(Ptr, Type, N_items) \
+  ((Type *) xrealloc ((void *) (Ptr), sizeof (Type) * (N_items)))
 
 /* Declare and alloc memory for VAR of type TYPE. */
 # define NEW(Type, Var)  Type *(Var) = XMALLOC (Type, 1)
diff -pru coreutils/lib/xmalloc.c coreutils-strcoll/lib/xmalloc.c
--- coreutils/lib/xmalloc.c     Tue Jun 17 23:14:01 2003
+++ coreutils-strcoll/lib/xmalloc.c     Thu Jul 17 11:30:49 2003
@@ -37,6 +37,7 @@ void free ();
 #define N_(msgid) msgid
 
 #include "error.h"
+#include "exitfail.h"
 #include "xalloc.h"
 
 #ifndef EXIT_FAILURE
@@ -53,10 +54,6 @@ void free ();
 "you must run the autoconf test for a GNU libc compatible realloc"
 #endif
 
-/* Exit value when the requested amount of memory is not available.
-   The caller may set it to some other value.  */
-int xalloc_exit_failure = EXIT_FAILURE;
-
 /* If non NULL, call this function when memory is exhausted. */
 void (*xalloc_fail_func) (void) = 0;
 
@@ -69,7 +66,7 @@ xalloc_die (void)
 {
   if (xalloc_fail_func)
     (*xalloc_fail_func) ();
-  error (xalloc_exit_failure, 0, "%s", _(xalloc_msg_memory_exhausted));
+  error (exit_failure, 0, "%s", _(xalloc_msg_memory_exhausted));
   /* The `noreturn' cannot be given to error, since it may return if
      its first argument is 0.  To help compilers understand the
      xalloc_die does terminate, call exit. */
diff -pru coreutils/lib/xmemcoll.c coreutils-strcoll/lib/xmemcoll.c
--- coreutils/lib/xmemcoll.c    Sat Nov 23 07:44:51 2002
+++ coreutils-strcoll/lib/xmemcoll.c    Thu Jul 17 11:31:20 2003
@@ -32,14 +32,11 @@ extern int errno;
 #define _(msgid) gettext (msgid)
 
 #include "error.h"
+#include "exitfail.h"
 #include "memcoll.h"
 #include "quotearg.h"
 #include "xmemcoll.h"
 
-/* Exit value when xmemcoll fails.
-   The caller may set it to some other value.  */
-int xmemcoll_exit_failure = EXIT_FAILURE;
-
 /* Compare S1 (with length S1LEN) and S2 (with length S2LEN) according
    to the LC_COLLATE locale.  S1 and S2 do not overlap, and are not
    adjacent.  Temporarily modify the bytes after S1 and S2, but
@@ -56,7 +53,7 @@ xmemcoll (char *s1, size_t s1len, char *
     {
       error (0, collation_errno, _("string comparison failed"));
       error (0, 0, _("Set LC_ALL='C' to work around the problem."));
-      error (xmemcoll_exit_failure, 0,
+      error (exit_failure, 0,
             _("The strings compared were %s and %s."),
             quotearg_n_style_mem (0, locale_quoting_style, s1, s1len),
             quotearg_n_style_mem (1, locale_quoting_style, s2, s2len));
diff -pru coreutils/lib/xmemcoll.h coreutils-strcoll/lib/xmemcoll.h
--- coreutils/lib/xmemcoll.h    Sat Jun  7 03:11:39 2003
+++ coreutils-strcoll/lib/xmemcoll.h    Thu Jul 17 11:31:50 2003
@@ -1,3 +1,2 @@
 #include <stddef.h>
-extern int xmemcoll_exit_failure;
 int xmemcoll (char *, size_t, char *, size_t);
diff -pru coreutils/src/expr.c coreutils-strcoll/src/expr.c
--- coreutils/src/expr.c        Tue Jun 17 11:13:23 2003
+++ coreutils-strcoll/src/expr.c        Thu Jul 17 14:36:11 2003
@@ -37,7 +37,9 @@
 #include "long-options.h"
 #include "error.h"
 #include "closeout.h"
+#include "exitfail.h"
 #include "inttostr.h"
+#include "quotearg.h"
 
 /* The official name of this program (e.g., no `g' prefix).  */
 #define PROGRAM_NAME "expr"
@@ -48,6 +50,18 @@
 #define NEW(Type) XMALLOC (Type, 1)
 #define OLD(x) free (x)
 
+/* Exit statuses.  */
+enum
+  {
+    /* Invalid expression: i.e., its form does not conform to the
+       grammar for expressions.  Our grammar is an extension of the
+       POSIX grammar.  */
+    EXPR_INVALID = 2,
+
+    /* Some other error occurred.  */
+    EXPR_ERROR
+  };
+
 /* The kinds of value we can have.  */
 enum valtype
 {
@@ -75,8 +89,8 @@ static char **args;
 char *program_name;
 
 static VALUE *eval (void);
-static int nomoreargs (void);
-static int null (VALUE *v);
+static bool nomoreargs (void);
+static bool null (VALUE *v);
 static void printv (VALUE *v);
 
 void
@@ -151,6 +165,13 @@ Pattern matches return the string matche
   exit (status);
 }
 
+/* Report a syntax error and exit.  */
+static void
+syntax_error (void)
+{
+  error (EXPR_INVALID, 0, _("syntax error"));
+}
+
 int
 main (int argc, char **argv)
 {
@@ -162,6 +183,9 @@ main (int argc, char **argv)
   bindtextdomain (PACKAGE, LOCALEDIR);
   textdomain (PACKAGE);
 
+  /* Change the way library functions fail.  */
+  exit_failure = EXPR_ERROR;
+
   atexit (close_stdout);
 
   parse_long_options (argc, argv, PROGRAM_NAME, GNU_PACKAGE, VERSION,
@@ -177,14 +201,14 @@ main (int argc, char **argv)
   if (argc <= 1)
     {
       error (0, 0, _("too few arguments"));
-      usage (EXIT_FAILURE);
+      usage (EXPR_INVALID);
     }
 
   args = argv + 1;
 
   v = eval ();
   if (!nomoreargs ())
-    error (2, 0, _("syntax error"));
+    syntax_error ();
   printv (v);
 
   exit (null (v));
@@ -249,9 +273,9 @@ printv (VALUE *v)
   puts (p);
 }
 
-/* Return nonzero if V is a null-string or zero-number.  */
+/* Return true if V is a null-string or zero-number.  */
 
-static int
+static bool
 null (VALUE *v)
 {
   switch (v->type)
@@ -285,19 +309,19 @@ tostring (VALUE *v)
     }
 }
 
-/* Coerce V to an integer value.  Return 1 on success, 0 on failure.  */
+/* Coerce V to an integer value.  Return true on success, false on failure.  */
 
-static int
+static bool
 toarith (VALUE *v)
 {
   intmax_t i;
-  int neg;
+  bool neg;
   char *cp;
 
   switch (v->type)
     {
     case integer:
-      return 1;
+      return true;
     case string:
       i = 0;
       cp = v->u.s;
@@ -310,14 +334,14 @@ toarith (VALUE *v)
          if (ISDIGIT (*cp))
            i = i * 10 + *cp - '0';
          else
-           return 0;
+           return false;
        }
       while (*++cp);
 
       free (v->u.s);
       v->u.i = i * (neg ? -1 : 1);
       v->type = integer;
-      return 1;
+      return true;
     default:
       abort ();
     }
@@ -326,22 +350,22 @@ toarith (VALUE *v)
 /* Return nonzero and advance if the next token matches STR exactly.
    STR must not be NULL.  */
 
-static int
-nextarg (char *str)
+static bool
+nextarg (char const *str)
 {
   if (*args == NULL)
     return 0;
   else
     {
-      int r = strcoll (*args, str) == 0;
+      bool r = strcmp (*args, str) == 0;
       args += r;
       return r;
     }
 }
 
-/* Return nonzero if there no more tokens.  */
+/* Return true if there no more tokens.  */
 
-static int
+static bool
 nomoreargs (void)
 {
   return *args == 0;
@@ -399,7 +423,7 @@ of the basic regular expression is not p
   re_syntax_options = RE_SYNTAX_POSIX_BASIC;
   errmsg = re_compile_pattern (pv->u.s, len, &re_buffer);
   if (errmsg)
-    error (2, 0, "%s", errmsg);
+    error (EXPR_ERROR, 0, "%s", errmsg);
 
   matchlen = re_match (&re_buffer, sv->u.s, strlen (sv->u.s), 0, &re_regs);
   if (0 <= matchlen)
@@ -436,18 +460,18 @@ eval7 (void)
   trace ("eval7");
 #endif
   if (nomoreargs ())
-    error (2, 0, _("syntax error"));
+    syntax_error ();
 
   if (nextarg ("("))
     {
       v = eval ();
       if (!nextarg (")"))
-       error (2, 0, _("syntax error"));
+       syntax_error ();
       return v;
     }
 
   if (nextarg (")"))
-    error (2, 0, _("syntax error"));
+    syntax_error ();
 
   return str_value (*args++);
 }
@@ -469,7 +493,7 @@ eval6 (void)
   if (nextarg ("+"))
     {
       if (nomoreargs ())
-       error (2, 0, _("syntax error"));
+       syntax_error ();
       return str_value (*args++);
     }
   else if (nextarg ("length"))
@@ -584,13 +608,13 @@ eval4 (void)
        return l;
       r = eval5 ();
       if (!toarith (l) || !toarith (r))
-       error (2, 0, _("non-numeric argument"));
+       error (EXPR_ERROR, 0, _("non-numeric argument"));
       if (fxn == multiply)
        val = l->u.i * r->u.i;
       else
        {
          if (r->u.i == 0)
-           error (2, 0, _("division by zero"));
+           error (EXPR_ERROR, 0, _("division by zero"));
          val = fxn == divide ? l->u.i / r->u.i : l->u.i % r->u.i;
        }
       freev (l);
@@ -623,7 +647,7 @@ eval3 (void)
        return l;
       r = eval4 ();
       if (!toarith (l) || !toarith (r))
-       error (2, 0, _("non-numeric argument"));
+       error (EXPR_ERROR, 0, _("non-numeric argument"));
       val = fxn == plus ? l->u.i + r->u.i : l->u.i - r->u.i;
       freev (l);
       freev (r);
@@ -642,9 +666,11 @@ eval2 (void)
   {
     less_than, less_equal, equal, not_equal, greater_equal, greater_than
   } fxn;
-  int val;
+  bool val;
   intmax_t lval;
   intmax_t rval;
+  int collation_errno;
+  char *collation_arg1;
 
 #ifdef EVAL_TRACE
   trace ("eval2");
@@ -669,13 +695,31 @@ eval2 (void)
       r = eval3 ();
       tostring (l);
       tostring (r);
-      lval = strcoll (l->u.s, r->u.s);
+
+      /* Save the first arg to strcoll, in case we need its value for
+        a diagnostic later.  This is needed because 'toarith' might
+        free the first arg.  */
+      collation_arg1 = xstrdup (l->u.s);
+
+      errno = 0;
+      lval = strcoll (collation_arg1, r->u.s);
+      collation_errno = errno;
       rval = 0;
       if (toarith (l) && toarith (r))
        {
          lval = l->u.i;
          rval = r->u.i;
        }
+      else if (collation_errno)
+       {
+         error (0, collation_errno, _("string comparison failed"));
+         error (0, 0, _("Set LC_ALL='C' to work around the problem."));
+         error (EXPR_ERROR, 0,
+                _("The strings compared were %s and %s."),
+                quotearg_n_style (0, locale_quoting_style, collation_arg1),
+                quotearg_n_style (1, locale_quoting_style, r->u.s));
+       }
+
       switch (fxn)
        {
        case less_than:     val = (lval <  rval); break;
@@ -688,6 +732,7 @@ eval2 (void)
        }
       freev (l);
       freev (r);
+      free (collation_arg1);
       l = int_value (val);
     }
 }
diff -pru coreutils/src/sort.c coreutils-strcoll/src/sort.c
--- coreutils/src/sort.c        Tue Jun 17 11:13:24 2003
+++ coreutils-strcoll/src/sort.c        Thu Jul 17 13:27:34 2003
@@ -31,6 +31,7 @@
 #include "system.h"
 #include "long-options.h"
 #include "error.h"
+#include "exitfail.h"
 #include "hard-locale.h"
 #include "inttostr.h"
 #include "physmem.h"
@@ -2209,8 +2210,7 @@ main (int argc, char **argv)
   inittables ();
 
   /* Change the way library functions fail.  */
-  xalloc_exit_failure = SORT_FAILURE;
-  xmemcoll_exit_failure = SORT_FAILURE;
+  exit_failure = SORT_FAILURE;
 
 #ifdef SA_NOCLDSTOP
   {
diff -pru coreutils/tests/expr/basic coreutils-strcoll/tests/expr/basic
--- coreutils/tests/expr/basic  Tue Apr  8 03:55:01 2003
+++ coreutils-strcoll/tests/expr/basic  Thu Jul 17 14:07:45 2003
@@ -44,7 +44,7 @@ my @Tests =
 
      # This erroneously succeeded and output `3' before 2.0.12.
      ['fail-a', '3 + -', {ERR => "$prog: non-numeric argument\n"},
-      {EXIT => 2}],
+      {EXIT => 3}],
     );
 
 # Append a newline to end of each expected `OUT' string.