From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16822 invoked by alias); 22 Jan 2006 00:31:22 -0000 Received: (qmail 16812 invoked by uid 22791); 22 Jan 2006 00:31:21 -0000 X-Spam-Check-By: sourceware.org Received: from nevyn.them.org (HELO nevyn.them.org) (66.93.172.17) by sourceware.org (qpsmtpd/0.31.1) with ESMTP; Sun, 22 Jan 2006 00:31:19 +0000 Received: from drow by nevyn.them.org with local (Exim 4.54) id 1F0T8X-0002sI-5J for gdb-patches@sourceware.org; Sat, 21 Jan 2006 19:31:17 -0500 Date: Sun, 22 Jan 2006 00:31:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sourceware.org Subject: RFC: Fix various problems with "printf" and warnings Message-ID: <20060122003117.GC8088@nevyn.them.org> Mail-Followup-To: gdb-patches@sourceware.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.8i X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2006-01/txt/msg00285.txt.bz2 This patch solves the same general problem three different ways: the annoying warnings while building printcmd.o and the underlying problem they represent. First, a Debian user pointed out that calling "nomem" because vasprintf failed isn't a good choice. printf is allowed to fail for other reasons, including bad format strings - and sometimes it does. Second, the existing code passes a lot of bogus format strings on to the system printf. If we're going to suppress the warnings about our use of user-provided format strings we at least ought to sanity check them better. So this patch overhauls printf_command to match ISO C89, with the exception of long long support (autoconf enabled; I didn't touch that bit). Third, the right way to build this file is to remove -Wformat-nonliteral now that we're convinced the warnings do not represent a real problem. Any comments on, or objections to, this patch? -- Daniel Jacobowitz CodeSourcery 2006-01-21 Daniel Jacobowitz * printcmd.c (printf_command): Make format string checking stricter. Add separate cases for long_arg, ptr_arg, and long_double_arg. * utils.c (xstrvprintf): Improve the error message issued for a bad format string. * Makefile.in (GDB_WARN_CFLAGS_NO_FORMAT, INTERNAL_CFLAGS_BASE): New variables. (gnu-v3-abi.o, monitor.o, procfs.o, linux-thread-db.o): Remove $(NO_WERROR_CFLAGS). (printcmd.o): Likewise. Use $(GDB_WARN_CFLAGS_NO_FORMAT) and enable -Werror. Index: printcmd.c =================================================================== RCS file: /cvs/src/src/gdb/printcmd.c,v retrieving revision 1.96 diff -u -p -r1.96 printcmd.c --- printcmd.c 15 Jan 2006 19:50:03 -0000 1.96 +++ printcmd.c 22 Jan 2006 00:08:39 -0000 @@ -1836,13 +1836,13 @@ printf_command (char *arg, int from_tty) enum argclass { - no_arg, int_arg, string_arg, double_arg, long_long_arg + int_arg, long_arg, long_long_arg, ptr_arg, string_arg, + double_arg, long_double_arg }; enum argclass *argclass; enum argclass this_argclass; char *last_arg; int nargs_wanted; - int lcount; int i; argclass = (enum argclass *) alloca (strlen (s) * sizeof *argclass); @@ -1852,23 +1852,136 @@ printf_command (char *arg, int from_tty) while (*f) if (*f++ == '%') { - lcount = 0; - while (strchr ("0123456789.hlL-+ #", *f)) + int seen_hash = 0, seen_zero = 0, lcount = 0, seen_prec = 0; + int seen_space = 0, seen_plus = 0; + int seen_big_l = 0, seen_h = 0; + int bad = 0; + + /* Check the validity of the format specifier, and work + out what argument it expects. We only accept C89 + format strings, with the exception of long long (which + we autoconf for). */ + + /* Skip over "%%". */ + if (*f == '%') + { + f++; + continue; + } + + /* The first part of a format specifier is a set of flag + characters. */ + while (strchr ("0-+ #", *f)) + { + if (*f == '#') + seen_hash = 1; + else if (*f == '0') + seen_zero = 1; + else if (*f == ' ') + seen_space = 1; + else if (*f == ' ') + seen_plus = 1; + f++; + } + + /* The next part of a format specifier is a width. */ + while (strchr ("0123456789", *f)) + f++; + + /* The next part of a format specifier is a precision. */ + if (*f == '.') + { + seen_prec = 1; + f++; + while (strchr ("0123456789", *f)) + f++; + } + + /* The next part of a format specifier is a length modifier. */ + if (*f == 'h') { - if (*f == 'l' || *f == 'L') - lcount++; + seen_h = 1; f++; } + else if (*f == 'l') + { + f++; + lcount++; + if (*f == 'l') + { + f++; + lcount++; + } + } + else if (*f == 'L') + { + seen_big_l = 1; + f++; + } + switch (*f) { + case 'u': + if (seen_hash) + bad = 1; + /* FALLTHROUGH */ + + case 'o': + case 'x': + case 'X': + if (seen_space || seen_plus) + bad = 1; + /* FALLTHROUGH */ + + case 'd': + case 'i': + if (lcount == 0) + this_argclass = int_arg; + else if (lcount == 1) + this_argclass = long_arg; + else + this_argclass = long_long_arg; + + if (seen_big_l) + bad = 1; + break; + + case 'c': + this_argclass = int_arg; + if (lcount || seen_h || seen_big_l) + bad = 1; + if (seen_prec || seen_zero || seen_space || seen_plus) + bad = 1; + break; + + case 'p': + this_argclass = ptr_arg; + if (lcount || seen_h || seen_big_l) + bad = 1; + if (seen_prec || seen_zero || seen_space || seen_plus) + bad = 1; + break; + case 's': this_argclass = string_arg; + if (lcount || seen_h || seen_big_l) + bad = 1; + if (seen_zero || seen_space || seen_plus) + bad = 1; break; case 'e': case 'f': case 'g': - this_argclass = double_arg; + case 'E': + case 'G': + if (seen_big_l) + this_argclass = long_double_arg; + else + this_argclass = double_arg; + + if (lcount || seen_h) + bad = 1; break; case '*': @@ -1877,26 +1990,23 @@ printf_command (char *arg, int from_tty) case 'n': error (_("Format specifier `n' not supported in printf")); - case '%': - this_argclass = no_arg; - break; + case '\0': + error (_("Incomplete format specifier at end of format string")); default: - if (lcount > 1) - this_argclass = long_long_arg; - else - this_argclass = int_arg; - break; + error (_("Unrecognized format specifier '%c' in printf"), *f); } + + if (bad) + error (_("Inappropriate modifiers to format specifier '%c' in printf"), + *f); + f++; - if (this_argclass != no_arg) - { - strncpy (current_substring, last_arg, f - last_arg); - current_substring += f - last_arg; - *current_substring++ = '\0'; - last_arg = f; - argclass[nargs_wanted++] = this_argclass; - } + strncpy (current_substring, last_arg, f - last_arg); + current_substring += f - last_arg; + *current_substring++ = '\0'; + last_arg = f; + argclass[nargs_wanted++] = this_argclass; } /* Now, parse all arguments and evaluate them. @@ -1970,6 +2080,16 @@ printf_command (char *arg, int from_tty) printf_filtered (current_substring, val); break; } + case long_double_arg: +#ifdef HAVE_LONG_DOUBLE + { + long double val = value_as_double (val_args[i]); + printf_filtered (current_substring, val); + break; + } +#else + error (_("long double not supported in printf")); +#endif case long_long_arg: #if defined (CC_HAS_LONG_LONG) && defined (PRINTF_HAS_LONG_LONG) { @@ -1982,7 +2102,12 @@ printf_command (char *arg, int from_tty) #endif case int_arg: { - /* FIXME: there should be separate int_arg and long_arg. */ + int val = value_as_long (val_args[i]); + printf_filtered (current_substring, val); + break; + } + case long_arg: + { long val = value_as_long (val_args[i]); printf_filtered (current_substring, val); break; Index: utils.c =================================================================== RCS file: /cvs/src/src/gdb/utils.c,v retrieving revision 1.163 diff -u -p -r1.163 utils.c --- utils.c 17 Dec 2005 22:34:03 -0000 1.163 +++ utils.c 22 Jan 2006 00:09:48 -0000 @@ -1068,14 +1068,12 @@ xstrvprintf (const char *format, va_list { char *ret = NULL; int status = vasprintf (&ret, format, ap); - /* NULL is returned when there was a memory allocation problem. */ - if (ret == NULL) - nomem (0); - /* A negative status (the printed length) with a non-NULL buffer - should never happen, but just to be sure. */ - if (status < 0) - internal_error (__FILE__, __LINE__, - _("vasprintf call failed (errno %d)"), errno); + /* NULL is returned when there was a memory allocation problem, or + any other error (for instance, a bad format string). A negative + status (the printed length) with a non-NULL buffer should never + happen, but just to be sure. */ + if (ret == NULL || status < 0) + internal_error (__FILE__, __LINE__, _("vasprintf call failed")); return ret; } Index: Makefile.in =================================================================== RCS file: /cvs/src/src/gdb/Makefile.in,v retrieving revision 1.777 diff -u -p -r1.777 Makefile.in --- Makefile.in 21 Jan 2006 01:29:03 -0000 1.777 +++ Makefile.in 22 Jan 2006 00:22:53 -0000 @@ -132,6 +132,8 @@ WERROR_CFLAGS = @WERROR_CFLAGS@ GDB_WARN_CFLAGS = $(WARN_CFLAGS) GDB_WERROR_CFLAGS = $(WERROR_CFLAGS) +GDB_WARN_CFLAGS_NO_FORMAT = `echo $(GDB_WARN_CFLAGS) | sed s/-Wformat-nonliteral//` + # Where is the INTL library? Typically in ../intl. INTL_DIR = ../intl INTL = @INTLLIBS@ @@ -344,12 +346,12 @@ CFLAGS = @CFLAGS@ CXXFLAGS = -g -O # INTERNAL_CFLAGS is the aggregate of all other *CFLAGS macros. -INTERNAL_WARN_CFLAGS = \ +INTERNAL_CFLAGS_BASE = \ $(CFLAGS) $(GLOBAL_CFLAGS) $(PROFILE_CFLAGS) \ $(GDB_CFLAGS) $(OPCODES_CFLAGS) $(READLINE_CFLAGS) \ $(BFD_CFLAGS) $(INCLUDE_CFLAGS) \ - $(INTL_CFLAGS) $(ENABLE_CFLAGS) \ - $(GDB_WARN_CFLAGS) + $(INTL_CFLAGS) $(ENABLE_CFLAGS) +INTERNAL_WARN_CFLAGS = $(INTERNAL_CFLAGS_BASE) $(GDB_WARN_CFLAGS) INTERNAL_CFLAGS = $(INTERNAL_WARN_CFLAGS) $(GDB_WERROR_CFLAGS) # LDFLAGS is specifically reserved for setting from the command line @@ -1479,8 +1481,7 @@ main.o: main.c # return types - "enum gnu_v3_dtor_kinds" vs "enum ctor_kinds" - # conflict. gnu-v3-abi.o: $(srcdir)/gnu-v3-abi.c - $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) \ - $(srcdir)/gnu-v3-abi.c + $(CC) -c $(INTERNAL_WARN_CFLAGS) $(srcdir)/gnu-v3-abi.c # FIXME: cagney/2003-08-10: "monitor.c" gets -Wformat-nonliteral # errors. It turns out that that is the least of monitor.c's @@ -1489,25 +1490,23 @@ gnu-v3-abi.o: $(srcdir)/gnu-v3-abi.c # definitly will not work. "monitor.c" needs to be rewritten so that # it doesn't use format strings and instead uses callbacks. monitor.o: $(srcdir)/monitor.c - $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) $(srcdir)/monitor.c + $(CC) -c $(INTERNAL_WARN_CFLAGS) $(srcdir)/monitor.c -# FIXME: cagney/2003-08-10: Do not try to build "printcmd.c" with -# -Wformat-nonliteral. It needs to be overhauled so that it doesn't -# pass user input strings as the format parameter to host printf -# function calls. +# Do not try to build "printcmd.c" with -Wformat-nonliteral. It manually +# checks format strings. printcmd.o: $(srcdir)/printcmd.c - $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) $(srcdir)/printcmd.c + $(CC) -c $(INTERNAL_CFLAGS_BASE) $(GDB_WARN_CFLAGS_NO_FORMAT) \ + $(GDB_WERROR_CFLAGS) $(srcdir)/printcmd.c # FIXME: Procfs.o gets -Wformat errors because things like pid_t don't # match output format strings. procfs.o: $(srcdir)/procfs.c - $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) $(srcdir)/procfs.c + $(CC) -c $(INTERNAL_WARN_CFLAGS) $(srcdir)/procfs.c # FIXME: Thread-db.o gets warnings because the definitions of the register # sets are different from kernel to kernel. linux-thread-db.o: $(srcdir)/linux-thread-db.c - $(CC) -c $(INTERNAL_WARN_CFLAGS) $(NO_WERROR_CFLAGS) \ - $(srcdir)/linux-thread-db.c + $(CC) -c $(INTERNAL_WARN_CFLAGS) $(srcdir)/linux-thread-db.c v850ice.o: $(srcdir)/v850ice.c $(CC) -c $(INTERNAL_CFLAGS) $(IDE_CFLAGS) $(ITCL_CFLAGS) \