[PATCH] Don't call strchr with the NULL character.

[PATCH] Don't call strchr with the NULL character.

[Andrew Burgess]

In the printf code we call strchr without guarding against the
case where the second parameter is NULL.

My local manpage for strchr doesn't say what happens in this case,
but this file:  src/libiberty/strchr.c
suggests the results are undefined, and indeed, the answer I see is
not NULL (which is what I might have hoped for).

Patch below adds check for NULL character, and some tests which are
currently failing for me, but as it's string overflow, the results 
are going to be undefined.

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

	* common/format.c (parse_format_string): Add checks for NULL
	character before calling strchr.


gdb/testsuite/ChangeLog

	* gdb.base/printcmds.exp (test_printf): Add tests for format
	strings with missing format specifier.


diff --git a/gdb/common/format.c b/gdb/common/format.c
index 5803818..1bdd253 100644
--- a/gdb/common/format.c
+++ b/gdb/common/format.c
@@ -156,7 +156,7 @@ parse_format_string (const char **arg)
 
 	/* The first part of a format specifier is a set of flag
 	   characters.  */
-	while (strchr ("0-+ #", *f))
+	while (*f != '\0' && strchr ("0-+ #", *f))
 	  {
 	    if (*f == '#')
 	      seen_hash = 1;
@@ -170,7 +170,7 @@ parse_format_string (const char **arg)
 	  }
 
 	/* The next part of a format specifier is a width.  */
-	while (strchr ("0123456789", *f))
+	while (*f != '\0' && strchr ("0123456789", *f))
 	  f++;
 
 	/* The next part of a format specifier is a precision.  */
@@ -178,7 +178,7 @@ parse_format_string (const char **arg)
 	  {
 	    seen_prec = 1;
 	    f++;
-	    while (strchr ("0123456789", *f))
+	    while (*f != '\0' && strchr ("0123456789", *f))
 	      f++;
 	  }
 
diff --git a/gdb/testsuite/gdb.base/printcmds.exp b/gdb/testsuite/gdb.base/printcmds.exp
index 4883fd5..4f88382 100644
--- a/gdb/testsuite/gdb.base/printcmds.exp
+++ b/gdb/testsuite/gdb.base/printcmds.exp
@@ -728,6 +728,12 @@ proc test_printf {} {
     # Regression test for "%% at end of format string.
     # See http://sourceware.org/bugzilla/show_bug.cgi?id=11345
     gdb_test "printf \"%%%d%%\\n\", 5" "%5%"
+
+    # Some tests for missing format specifier after '%'.
+    gdb_test "printf \"%\", 0" "Incomplete format specifier at end of format string"
+    gdb_test "printf \"%.234\", 0" "Incomplete format specifier at end of format string"
+    gdb_test "printf \"%-\", 0" "Incomplete format specifier at end of format string"
+    gdb_test "printf \"%-23\", 0" "Incomplete format specifier at end of format string"
 }
 
 #Test printing DFP values with printf

Re: [PATCH] Don't call strchr with the NULL character.

[Andreas Schwab]

"Andrew Burgess" <aburgess@broadcom.com> writes:

> My local manpage for strchr doesn't say what happens in this case,

7.24.5.2 The strchr function
... The terminating null character is considered to be part of the
string.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Don't call strchr with the NULL character.

[Paul_Koning]

On Jul 11, 2013, at 12:15 PM, Andrew Burgess wrote:

> In the printf code we call strchr without guarding against the
> case where the second parameter is NULL.
> 
> My local manpage for strchr doesn't say what happens in this case,
> but this file:  src/libiberty/strchr.c
> suggests the results are undefined, and indeed, the answer I see is
> not NULL (which is what I might have hoped for).

The BSD manpage (for example on Mac OS) says:
     The strchr() function locates the first occurrence of c (converted to a
     char) in the string pointed to by s.  The terminating null character is
     considered to be part of the string; therefore if c is `\0', the func-
     tions locate the terminating `\0'.

It looks like libiberty/strchr.c does the same thing.  It doesn't look undefined at all. 

	paul

Re: [PATCH] Don't call strchr with the NULL character.

[Andrew Burgess]

On 11/07/2013 5:15 PM, Andrew Burgess wrote:
> In the printf code we call strchr without guarding against the
> case where the second parameter is NULL.
> 
> My local manpage for strchr doesn't say what happens in this case,
> but this file:  src/libiberty/strchr.c
> suggests the results are undefined, and indeed, the answer I see is
> not NULL (which is what I might have hoped for).

Thanks to both Andreas and Paul for pointing out more up to
date manual pages that explain the behaviour is NOT undefined
at all.

That said, my patch (I believe) fixes gdb given the /current/
behaviour, which I thought was undefined, but is in fact well
defined.  Either way I believe this patch is required.

Here's an example from before my patch:

	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++;
	  }

If *f is the end of string NULL character then the loop
above will run off the end of the string.


Thanks,
Andrew

Re: [PATCH] Don't call strchr with the NULL character.

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@broadcom.com> writes:

Andrew> That said, my patch (I believe) fixes gdb given the /current/
Andrew> behaviour, which I thought was undefined, but is in fact well
Andrew> defined.  Either way I believe this patch is required.

I agree.  The patch is ok.

thanks,
Tom

Re: [PATCH] Don't call strchr with the NULL character.

[Andrew Burgess]

On 16/07/2013 6:36 PM, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess <aburgess@broadcom.com> writes:
> 
> Andrew> That said, my patch (I believe) fixes gdb given the /current/
> Andrew> behaviour, which I thought was undefined, but is in fact well
> Andrew> defined.  Either way I believe this patch is required.
> 
> I agree.  The patch is ok.

Committed.

Thanks,
Andrew