Bug 81586 - valgrind error in output_buffer_append_r with -Wall
Summary: valgrind error in output_buffer_append_r with -Wall
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: c++ (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: ---
Assignee: Martin Sebor
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2017-07-27 16:52 UTC by David Binderman
Modified: 2017-09-13 16:50 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-07-27 00:00:00


Attachments
C++ source code (64.48 KB, text/plain)
2017-07-27 16:52 UTC, David Binderman
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Binderman 2017-07-27 16:52:26 UTC
Created attachment 41848 [details]
C++ source code

The attached C++ code, when compiled by recent gcc trunk and 
compiler flag -Wall, does this:

==19553== Conditional jump or move depends on uninitialised value(s)
==19553==    at 0x1227643: output_buffer_append_r (pretty-print.h:128)
==19553==    by 0x1227643: pp_append_r (pretty-print.c:258)
==19553==    by 0x1227643: pp_append_text(pretty_printer*, char const*, char con
st*) (pretty-print.c:892)
==19553==    by 0x12285E9: pp_format(pretty_printer*, text_info*) (pretty-print.
c:670)

The bug seems to have been created between revisions 243987 and 249539.
Comment 1 David Binderman 2017-07-27 17:04:34 UTC
The following much reduced C++ code seems to demonstrate the bug:

extern "C" int snprintf(char *, unsigned long, const char *...) ;
struct  S {
	char * a;
};
void f( S * af)
{
	snprintf(af->a, sizeof(af), "af_get_next_segv end of file reading segmen
t tail AFF file is truncated 0");
}
Comment 2 Martin Liška 2017-07-27 20:56:15 UTC
Confirmed, following patch will be needed:

diff --git a/gcc/gimple-ssa-sprintf.c b/gcc/gimple-ssa-sprintf.c
index 644cf7e33b1..72ec10a77b9 100644
--- a/gcc/gimple-ssa-sprintf.c
+++ b/gcc/gimple-ssa-sprintf.c
@@ -2500,7 +2500,7 @@ maybe_warn (substring_loc &dirloc, source_range *pargrange,
                  : G_("%<%.*s%> directive writing %wu bytes "
                       "into a region of size %wu")));
          return fmtwarn (dirloc, pargrange, NULL,
-                         info.warnopt (), fmtstr, dir.len,
+                         info.warnopt (), fmtstr, MIN (dir.len, 31),
                          target_to_host (hostdir, sizeof hostdir, dir.beg),
                          res.min, navail);
        }

I'm not sure about the constant (maybe 32) and there are multiple invocations of the fmtwarn function. Leaving to Martin.
Comment 3 Martin Sebor 2017-07-27 21:42:46 UTC
I don't see a problem with the code in maybe_warn.  It does this:

  /* Buffer for the directive in the host character set (used when
     the source character set is different).  */
  char hostdir[32];
  ...
          return fmtwarn (dirloc, pargrange, NULL,
                          info.warnopt (), fmtstr, dir.len,
                          target_to_host (hostdir, sizeof hostdir, dir.beg),
                          res.min, navail);

The call being made has fmtstr = "%<%.*s%> directive output truncated writing %wu bytes into a region of size %wu", dir.len = 73, and hostdir = "af_get_next_segv end of file..." (with strlen (hostdir) == 31).  With that, while the "%.*s" directive in fmtstr says to read at most 73 bytes from hostdir, since hostdir is only 31 characters long, the directive should read exactly that many and no more than that.

I think the bug is actually in pp_format where the "%.*s" directive is handled:

	case '.':
	  {
	    int n;
	    const char *s;

	    /* We handle '%.Ns' and '%.*s' or '%M$.*N$s'
	       (where M == N + 1).  The format string should be verified
	       already from the first phase.  */
	    p++;
	    if (ISDIGIT (*p))
	      {
		char *end;
		n = strtoul (p, &end, 10);
		p = end;
		gcc_assert (*p == 's');
	      }
	    else
	      {
		gcc_assert (*p == '*');
		p++;
		gcc_assert (*p == 's');
		n = va_arg (*text->args_ptr, int);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Here n is extracted from the variable argument list (the corresponding argument is dir.len).

		/* This consumes a second entry in the formatters array.  */
		gcc_assert (formatters[argno] == formatters[argno+1]);
		argno++;
	      }

	    s = va_arg (*text->args_ptr, const char *);
	    pp_append_text (pp, s, s + n);
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Here s points to hostdir and n is 73, but strlen(s) is just 31.  This code doesn't handle the precision correctly.  The precision is the maximum number of non-nul characters to print.  The fix is to set n to be no greater than strlen(s):

            if (strlen (s) < n)
              n = strlen (s);
	    pp_append_text (pp, s, s + n);
Comment 4 Martin Sebor 2017-07-27 21:50:25 UTC
Let me fix it.
Comment 5 Martin Sebor 2017-07-27 23:40:55 UTC
Patch: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01866.html
Comment 6 David Binderman 2017-08-09 15:36:26 UTC
(In reply to Martin Sebor from comment #5)
> Patch: https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01866.html

Did this patch ever get into trunk gcc ?

I have some evidence that gcc trunk revision 250947
doesn't have it.
Comment 7 Martin Sebor 2017-08-10 17:40:31 UTC
Fixed in r251029.
Comment 8 Martin Sebor 2017-08-10 17:40:43 UTC
Author: msebor
Date: Thu Aug 10 17:40:11 2017
New Revision: 251029

URL: https://gcc.gnu.org/viewcvs?rev=251029&root=gcc&view=rev
Log:
PR c++/81586 - valgrind error in output_buffer_append_r with -Wall

gcc/ChangeLog:

	PR c++/81586
	* pretty-print.c (pp_format): Correct the handling of %s precision.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/pretty-print.c
Comment 9 Aldy Hernandez 2017-09-13 16:50:32 UTC
Author: aldyh
Date: Wed Sep 13 16:50:00 2017
New Revision: 252389

URL: https://gcc.gnu.org/viewcvs?rev=252389&root=gcc&view=rev
Log:
PR c++/81586 - valgrind error in output_buffer_append_r with -Wall

gcc/ChangeLog:

	PR c++/81586
	* pretty-print.c (pp_format): Correct the handling of %s precision.

Modified:
    branches/range-gen2/gcc/ChangeLog
    branches/range-gen2/gcc/pretty-print.c