[Bug c++/88937] valgrind error in parse_has_include

2019-02-04 Thread dmalcolm at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88937

David Malcolm  changed:

   What|Removed |Added

  Component|preprocessor|c++
   Assignee|marxin at gcc dot gnu.org  |dmalcolm at gcc dot 
gnu.org
   Target Milestone|9.0 |10.0

--- Comment #5 from David Malcolm  ---
Patch:
  https://gcc.gnu.org/ml/gcc-patches/2019-02/msg00222.html

[Bug c++/88937] valgrind error in parse_has_include

2019-02-04 Thread dmalcolm at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88937

--- Comment #4 from David Malcolm  ---
It's clearly wrong to access token->val.node for token->type == CPP_STRING and
token->type == CPP_HEADER_NAME.  It's effectively casting the length of the
header name to a (cpp_hashnode *).

For reference, this was introduced in r215752 (2014-10-01) in gcc 5.

Patch review was:
  "[PATCH C++] - SD-6 Implementation Part 1 - __has_include."
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg00083.html

later revised to:
  "Re: [PATCH C++] - SD-6 Implementation Part 1 - __has_include."
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02533.html
as "patch_feature_test_1b"
  https://gcc.gnu.org/ml/gcc-patches/2014-09/txtMPDgWoRx_D.txt
  (approved by Jason here:
https://gcc.gnu.org/ml/gcc-patches/2014-09/msg02645.html )

The comment makes no sense in the given context.

It looks to me like this was copied-and-pasted from parse_defined, which also
has:

  /* A possible controlling macro of the form #if !defined ().
 _cpp_parse_expr checks there was no other junk on the line.  */
  pfile->mi_ind_cmacro = node;

where it *does* make sense; it's handling a CPP_NAME i.e. a "defined
(SOME_NAME)",
and recording the macro SOME_NAME, for handling that syntax for the
header-file-guard idiom.

Given that __has_include is purely checking for the presence of files (rather
than their inclusion), this seems meaningless here, and I think it's probably
best to delete that "node" handling from parse_has_include.

That said, I've been attempting and failing to turn this into a
read-through-the-corrupt-pointer crasher: _cpp_parse_expr, resets the
mi_ind_cmacro back, unless we have !has_include("foo.h")

1426  /* The controlling macro expression is only valid if we called lex 3
1427 times:   and .  push_conditional ()
1428 checks that we are at top-of-file.  */
1429  if (pfile->mi_ind_cmacro && !(saw_leading_not && lex_count == 3))
1430pfile->mi_ind_cmacro = 0;

and the "macro" is only saved in push_conditional if 

2130  if (pfile->mi_valid && pfile->mi_cmacro == 0)
2131ifs->mi_cmacro = cmacro;

and pfile->mi_valid is being cleared in my test cases.

Hence I don't *think* it's currently possible for the bogus pointer to be
dereferenced.

[Bug c++/88937] valgrind error in parse_has_include

2019-01-25 Thread marxin at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88937

Martin Liška  changed:

   What|Removed |Added

 CC||3dw4rd at verizon dot net,
   ||nathan at gcc dot gnu.org
   Target Milestone|--- |9.0

--- Comment #3 from Martin Liška  ---
CC'ing author of the commit and Nathan.

[Bug c++/88937] valgrind error in parse_has_include

2019-01-25 Thread marxin at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88937

Martin Liška  changed:

   What|Removed |Added

 CC||dmalcolm at gcc dot gnu.org

--- Comment #2 from Martin Liška  ---
So it's really problem, one can easily see it with:

$ cat x.C
#if __has_include("x")
#endif

$ valgrind --trace-children=yes ./xgcc -B. x.C  -c
==22322== Memcheck, a memory error detector
==22322== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==22322== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==22322== Command: ./xgcc -B. x.C -c
==22322== 
==22323== Memcheck, a memory error detector
==22323== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==22323== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==22323== Command: ./cc1plus -quiet -iprefix
/dev/shm/objdir/gcc/../lib64/gcc/x86_64-pc-linux-gnu/9.0.1/ -isystem ./include
-isystem ./include-fixed -D_GNU_SOURCE x.C -quiet -dumpbase x.C -mtune=generic
-march=x86-64 -auxbase x -o /tmp/ccBLUwdo.s
==22323== 
==22323== Conditional jump or move depends on uninitialised value(s)
==22323==at 0x18F99FA: parse_has_include(cpp_reader*, include_type)
(expr.c:2247)
==22323==by 0x18F6E81: eval_token(cpp_reader*, cpp_token const*, unsigned
int) (expr.c:1157)
==22323==by 0x18F717D: _cpp_parse_expr (expr.c:1328)
==22323==by 0x18F110C: do_if(cpp_reader*) (directives.c:2008)
==22323==by 0x18F27CA: _cpp_handle_directive (directives.c:543)
==22323==by 0x1901994: _cpp_lex_token (lex.c:2609)
==22323==by 0x19091D9: cpp_get_token_1(cpp_reader*, unsigned int*)
(macro.c:2703)
==22323==by 0x190964C: cpp_get_token_with_location(cpp_reader*, unsigned
int*) (macro.c:2889)
==22323==by 0xA9496E: c_lex_with_flags(tree_node**, unsigned int*, unsigned
char*, int) (c-lex.c:405)
==22323==by 0x95549E: cp_lexer_get_preprocessor_token(cp_lexer*, cp_token*)
(parser.c:788)
==22323==by 0x9925F7: cp_parser_initial_pragma (parser.c:40586)
==22323==by 0x9925F7: cp_lexer_new_main (parser.c:642)
==22323==by 0x9925F7: cp_parser_new (parser.c:3933)
==22323==by 0x9925F7: c_parse_file() (parser.c:41027)
==22323==by 0xA9E670: c_common_parse_file() (c-opts.c:1155)
==22323== 
==22323== Conditional jump or move depends on uninitialised value(s)
==22323==at 0x18F7408: _cpp_parse_expr (expr.c:1429)
==22323==by 0x18F110C: do_if(cpp_reader*) (directives.c:2008)
==22323==by 0x18F27CA: _cpp_handle_directive (directives.c:543)
==22323==by 0x1901994: _cpp_lex_token (lex.c:2609)
==22323==by 0x19091D9: cpp_get_token_1(cpp_reader*, unsigned int*)
(macro.c:2703)
==22323==by 0x190964C: cpp_get_token_with_location(cpp_reader*, unsigned
int*) (macro.c:2889)
==22323==by 0xA9496E: c_lex_with_flags(tree_node**, unsigned int*, unsigned
char*, int) (c-lex.c:405)
==22323==by 0x95549E: cp_lexer_get_preprocessor_token(cp_lexer*, cp_token*)
(parser.c:788)
==22323==by 0x9925F7: cp_parser_initial_pragma (parser.c:40586)
==22323==by 0x9925F7: cp_lexer_new_main (parser.c:642)
==22323==by 0x9925F7: cp_parser_new (parser.c:3933)
==22323==by 0x9925F7: c_parse_file() (parser.c:41027)
==22323==by 0xA9E670: c_common_parse_file() (c-opts.c:1155)
==22323==by 0xF55A8E: compile_file() (toplev.c:456)
==22323==by 0x856C19: do_compile (toplev.c:2176)
==22323==by 0x856C19: toplev::main(int, char**) (toplev.c:2311)

When putting following breakpoint:
(gdb) b expr.c:2218


you'll see:

$ (gdb) p token->val
$2 = {
  node = {
node = 0x3, 
spelling = 0x254eedc
  }, 
  source = 0x3, 
  str = {
len = 3, 
text = 0x254eedc "\"x\""
  }, 
  macro_arg = {
arg_no = 3, 
spelling = 0x254eedc
  }, 
  token_no = 3, 
  pragma = 3
}

so val.str is used from the union. Thus one shouldn't use
  node = token->val.node.node;

that will be 0x03. Later than the pointer is saved here:

  /* A possible controlling macro of the form #if !__has_include__ ().
 _cpp_parse_expr checks there was no other junk on the line.  */
  if (node)
pfile->mi_ind_cmacro = node;

David will you please take a look?

[Bug c++/88937] valgrind error in parse_has_include

2019-01-21 Thread marxin at gcc dot gnu.org
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88937

Martin Liška  changed:

   What|Removed |Added

 Status|UNCONFIRMED |ASSIGNED
   Last reconfirmed||2019-01-21
 CC||marxin at gcc dot gnu.org
   Assignee|unassigned at gcc dot gnu.org  |marxin at gcc dot 
gnu.org
 Ever confirmed|0   |1

--- Comment #1 from Martin Liška  ---
Confirmed, I can fix it.