[PATCH] locale: Fix localedef exit code [BZ #22292]

Carlos O'Donell carlos@redhat.com
Fri Oct 13 19:17:00 GMT 2017 

On 10/13/2017 06:19 AM, Florian Weimer wrote:
> On 10/13/2017 10:24 AM, Carlos O'Donell wrote:
> 
>> To fix this situation I have adopted the following high-level
>> changes:
>> * All errors are counted distinctly.
>> * All warnings are counted distinctly.
>> * All informative messages are not counted.
>> * Increasing verbosity cannot generate*more*  errors, and
>>    it previously did for errors conditional on verbose,
>>    this is now fixed.
>> * Increasing verbosity*can*  generate*more*  warnings.
>> * Making the output quiet cannot generate*less*  errors,
> 
> “fewer errors“ (in case this makes it into the commit message.

Fixed.

See attached patch which is git format-patch with the message
commit ready.

> The general approach of the patch looks okay to me.  I expect that
> one day, we might use the support/ infrastructure, potentially
> compiled for the host, in these programs as well, but that's not a
> current priority.

I agree completely. I was wondering if I could get this into support/
more easily, and then link that *.o file into all of localedef,
iconv, and locale.

If I did move it into support I think I'd want to avoid the global
modification and usage of the various parameters, and instead include
a reference to them passed to the record functions. I'd want to clean
it up even more, but I'm not at my threshold for doing this kind of
cleanup outside of locale/programs/, but at least this way we're half
way there to cleaning this up.

> 
>> @@ -1602,10 +1600,10 @@ collate_finish (struct localedef_t *locale, const struct charmap_t *charmap)
>>             {
>>               if (runp->weights[i].w[j]->weights == NULL)
>>                 {
>> -            WITH_CUR_LOCALE (error_at_line (0, 0, runp->file,
>> -                            runp->line,
>> -                            _("symbol `%s' not defined"),
>> -                            runp->weights[i].w[j]->name));
>> +            record_error_at_line (0, 0, runp->file,
>> +                          runp->line,
>> +                          _("symbol `%s' not defined"),
>> +                          runp->weights[i].w[j]->name);
> 
> runp->line fits on the preceding line (similar occurrences below).

Fixed. For ld-collate.c I fixed 4 instances where the line fit better moving
the arguments up.

>> @@ -1830,7 +1830,7 @@ symbol `%s' has the same encoding as"), (*eptr)->name);
>>         /* This seems not to be enforced by recent standards.  Don't
>>            emit an error, simply append UNDEFINED at the end.  */
>>         if (0)
>> -        WITH_CUR_LOCALE (error (0, 0, _("no definition of `UNDEFINED'")));
>> +        record_error (0, 0, _("no definition of `UNDEFINED'"));
> 
> Either turn this into a pure comment, or into a real warning.

Fixed. Nobody addes UNDEFINED at the end. We just add it ourselves in the tooling.
I tried enabling the error and almost all our locales error, so this can't be a
conservative thing to enable.

diff --git a/locale/programs/ld-collate.c b/locale/programs/ld-collate.c
index 79853fe..18df4e0 100644
--- a/locale/programs/ld-collate.c
+++ b/locale/programs/ld-collate.c
@@ -1824,10 +1824,6 @@ symbol `%s' has the same encoding as"), (*eptr)->name);
        {
          /* This seems not to be enforced by recent standards.  Don't
             emit an error, simply append UNDEFINED at the end.  */
-         if (0)
-           record_error (0, 0, _("no definition of `UNDEFINED'"));
-
-         /* Add UNDEFINED at the end.  */
          collate->undefined.mborder =
            (int *) obstack_alloc (&collate->mempool, nrules * sizeof (int));
--- 

>> diff --git a/locale/programs/linereader.h b/locale/programs/linereader.h
>> index 3965db5..2623802 100644
>> --- a/locale/programs/linereader.h
>> +++ b/locale/programs/linereader.h
> 
>> -#define lr_error(lr, fmt, args...) \
>> -  WITH_CUR_LOCALE (error_at_line (0, 0, lr->fname, lr->lineno, fmt, ## args))
>> -
>> +static inline void
>> +lr_error (struct linereader *lr, const char *fmt, ...)
>> +{
>> +  char *str;
>> +  va_list arg;
>> +  struct locale_state ls;
>> +  va_start (arg, fmt);
>> +  ls = push_locale ();
>> +  vasprintf (&str, fmt, arg);
>> +  pop_locale (ls);
>> +  va_end (arg);
>> +  error_at_line (0, 0, lr->fname, lr->lineno, str);
>> +  free (str);
>> +}
> 
> Format string bug: This needs to to use error_at_line (0, 0,
> lr->fname, lr->lineno, "%s", str).  Missing error checking, and
> missing format string attribute.

Fixed format string bug. Thanks for the security review.

Added abort() on vasprtinf failing.

Added format string attribute to help the compiler.

diff --git a/locale/programs/linereader.h b/locale/programs/linereader.h
index 2623802..f510b9a 100644
--- a/locale/programs/linereader.h
+++ b/locale/programs/linereader.h
@@ -22,6 +22,7 @@
 #include <libintl.h>
 #include <stdint.h>
 #include <stdio.h>
+#include <abort.h>
 
 #include "charmap.h"
 #include "error.h"
@@ -97,17 +98,21 @@ extern void lr_ignore_rest (struct linereader *lr, int verbose);
 
 
 static inline void
+__attribute__ ((__format__ (__printf__, 2, 3)))
 lr_error (struct linereader *lr, const char *fmt, ...)
 {
   char *str;
   va_list arg;
   struct locale_state ls;
+  int ret;
   va_start (arg, fmt); 
   ls = push_locale ();
-  vasprintf (&str, fmt, arg);
+  ret = vasprintf (&str, fmt, arg);
+  if (ret == -1)
+    abort ();
   pop_locale (ls);
   va_end (arg); 
-  error_at_line (0, 0, lr->fname, lr->lineno, str);
+  error_at_line (0, 0, lr->fname, lr->lineno, "%s", str);
   free (str);
 }
 
---

> 
>> diff --git a/locale/programs/record-status.h b/locale/programs/record-status.h
>> new file mode 100644
>> index 0000000..8701f8c
>> --- /dev/null
>> +++ b/locale/programs/record-status.h
>> @@ -0,0 +1,144 @@
>> +/* General definitions for recording error and warning status.
>> +   Copyright (C) 1998-2017 Free Software Foundation, Inc.
>> +   This file is part of the GNU C Library.
>> +
>> +   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; version 2 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/>.  */
>> +#ifndef _RECORD_STATUS_H
>> +#define _RECORD_STATUS_H 1
> 
> Missing empty line.

Fixed.

>> +/* The program using these functions must define these:  */
>> +extern int recorded_warning_count;
>> +extern int recorded_error_count;
>> +extern int be_quiet;
>> +extern int verbose;
> 
> You could turn these into tentative definitions (and update the
> comment).  Then the main program does not have to define them.

Fixed.
 
>> +/* Saved state of the current locale.  */
>> +struct locale_state
>> +{
>> +   const char *cur_locale;
>> +};
> 
> Drop the const for the change below.

OK.

>> +/* Alter the current locale to match the locale configured by the
>> +   user, and return the previous saved state.  */
>> +static inline struct locale_state
>> +push_locale (void)
>> +{
>> +  int saved_errno = errno;
>> +  const char *cl = setlocale (LC_CTYPE, NULL);
>> +  setlocale (LC_CTYPE, "");
>> +  errno = saved_errno;
>> +  return (struct locale_state) { .cur_locale = cl };
>> +}
>> +
>> +/* Use the saved state to restore the locale.  */
>> +static inline void
>> +pop_locale (struct locale_state ls)
>> +{
>> +  setlocale (LC_CTYPE, ls.cur_locale);
>> +}
> 
> The locale string returned by setlocale is only valid until the next
> setlocale call, I think, so you need to use strdup and free for the
> locale string.  strdup and setlocale need error checking.  Maybe
> introduce xsetlocale?

I don't want an xsetlocale, since the error is non-fatal to set the
locale for the error messages, you may be trying to use localedef
to recover from a corrupted or missing locale-archive.

See what I've done in v2.
- Used strdup
- Handled errors and printed messages via error, but don't
  abort the program.

>> +/* Wrapper to print verbose informative messages.  */
>> +static inline void
>> +record_verbose (FILE *stream, const char *format, ...)
>> +{
>> +  char *str;
>> +  va_list arg;
>> +
>> +  if (!verbose)
>> +    return;
>> +
>> +  if (!be_quiet)
>> +    {
>> +      struct locale_state ls;
>> +      va_start (arg, format);
>> +      ls = push_locale ();
>> +      vasprintf (&str, format, arg);
>> +      pop_locale (ls);
>> +      va_end (arg);
>> +      fprintf (stream, str);
>> +      free (str);
>> +    }
>> +}
> 
> These functions should not be inline and have printf/nonnull attributes.

Without inline the compiler will complain about unused functions. 
The point here is that these act like the macros they were replacing.
Unless you are suggesting we promote record-status.h to record-status.c
and do the work to link in a new object. At that point we should probably
just build a support object and link that in. I'd like to avoid this step
for today.

Added printf attribute for all functions.

Added nonull attribute for all functions.

> fprintf has a format string bug, should use fputs.  vasprintf needs error checking.

Fixed format string bug.

Switched to fputs.

Added error checking for all vasprintf (abort if -1 is returned).
 
>> +
>> +/* Wrapper to print warning messages.  We keep track of how
>> +   many were called because this effects our exit code.
>> +   Program must provide a definition of recorded_warning_count,
>> +   and be_quiet.  */
>> +static inline void
>> +record_warning (int status, int errnum, const char *format, ...)
>> +{
>> +  char *str;
>> +  va_list arg;
>> +  recorded_warning_count++;
>> +  if (!be_quiet)
>> +    {
>> +      struct locale_state ls;
>> +      va_start (arg, format);
>> +      ls = push_locale ();
>> +      vasprintf (&str, format, arg);
>> +      pop_locale (ls);
>> +      va_end (arg);
>> +      error (status, errnum, str);
>> +      free (str);
>> +    }
>> +}
> 
> The status/errnum parameters are unused and should be removed (status in particularly is conceptually unusable).

Agreed. Removed status/errnum and cleaned up all callers. Nothing set status/errnum to non-zero.

> error has a format string bug, should use error (status, errnum, "%s", str).

Fixed format string bug.

>> +/* Wrapper to print error messages.  We keep track of how
>> +   many were called because this effects our exit code.
>> +   Program must provide a definition of recorded_error_count
>> +   and be_quiet.  */
>> +static inline void
>> +record_error (int status, int errnum, const char *format, ...)
>> +{
>> +  char *str;
>> +  va_list arg;
>> +  recorded_error_count++;
>> +  if (!be_quiet)
>> +    {
>> +      struct locale_state ls;
>> +      va_start (arg, format);
>> +      ls = push_locale ();
>> +      vasprintf (&str, format, arg);
>> +      pop_locale (ls);
>> +      va_end (arg);
>> +      error (status, errnum, str);
>> +      free (str);
>> +    }
> 
> The condition needs to be !be_quiet || status != 0, otherwise be_quiet also disables fatal errors.  This applies to record_error_at_line, too.

Fixed this differently.

Even fatal errors should be quiet of --quiet is used, that's the purpose.

Added:

if (status != 0)
  exit (status)

v2 attached.

Question: Should locale/programs/locale.c 'static int verbose;' become 'int verbose'?

-- 
Cheers,
Carlos.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-locale-Fix-localedef-exit-code-Bug-22292.patch
Type: text/x-patch
Size: 89148 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libc-alpha/attachments/20171013/96ac88c6/attachment.bin>

More information about the Libc-alpha mailing list