* [PATCH][gdb] Fix heap-use-after-free in typename_concat
@ 2019-05-03 9:31 Tom de Vries
2019-05-16 8:17 ` [PING][PATCH][gdb] " Tom de Vries
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Tom de Vries @ 2019-05-03 9:31 UTC (permalink / raw)
To: gdb-patches
Hi,
When running gdb using AddressSanitizer, and loading a cc1plus binary built
with profiledbootstrap and -flto, we run into a heap-use-after-free error:
...
$ LD_PRELOAD=/usr/lib64/libasan.so.3 ./gdb -batch cc1plus
==26855==ERROR: AddressSanitizer: heap-use-after-free on address \
0x62100ad8a8b0 at pc 0x7f13803cc9e3 bp 0x7ffe55b0d090 sp 0x7ffe55b0c840
READ of size 47 at 0x62100ad8a8b0 thread T0
#0 0x7f13803cc9e2 (/usr/lib64/libasan.so.3+0x3e9e2)
#1 0x5e7a0d in typename_concat gdb/dwarf2read.c:22661
#2 0x5c6437 in partial_die_full_name gdb/dwarf2read.c:8876
#3 0x5c6555 in add_partial_symbol gdb/dwarf2read.c:8893
#4 0x5c6ecf in add_partial_subprogram gdb/dwarf2read.c:9156
#5 0x5c5e90 in scan_partial_symbols gdb/dwarf2read.c:8668
#6 0x5c6c0a in add_partial_namespace gdb/dwarf2read.c:9081
#7 0x5c5f99 in scan_partial_symbols gdb/dwarf2read.c:8702
#8 0x5c48b6 in process_psymtab_comp_unit_reader gdb/dwarf2read.c:8056
#9 0x5c3c1f in init_cutu_and_read_dies gdb/dwarf2read.c:7689
#10 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
#11 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
#12 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
#13 0x612359 in read_psyms gdb/elfread.c:1311
#14 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
#15 0x867d7b in read_symbols gdb/symfile.c:821
#16 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
#17 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
#18 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
#19 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
enum_flags<symfile_add_flag>, std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
#20 0x868b64 in symbol_file_add(char const*, \
enum_flags<symfile_add_flag>, \
std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>) gdb/symfile.c:1217
#21 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
#22 0x868bd0 in symbol_file_add_main(char const*, \
enum_flags<symfile_add_flag>) gdb/symfile.c:1231
#23 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
#24 0x71f10e in catch_command_errors gdb/main.c:372
#25 0x71ff5f in captured_main_1 gdb/main.c:1043
#26 0x72045d in captured_main gdb/main.c:1163
#27 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
#28 0x40fd7d in main gdb/gdb.c:32
#29 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
#30 0x40fc89 in _start (/data/gdb_versions/devel/build/gdb/gdb+0x40fc89)
0x62100ad8a8b0 is located 944 bytes inside of 4064-byte region \
[0x62100ad8a500,0x62100ad8b4e0)
freed by thread T0 here:
#0 0x7f13804523a0 in __interceptor_free (/usr/lib64/libasan.so.3+0xc43a0)
#1 0x435e44 in xfree<void> gdb/common/common-utils.h:60
#2 0xa82c25 in call_freefun libiberty/obstack.c:103
#3 0xa83098 in _obstack_free libiberty/obstack.c:280
#4 0x4367da in auto_obstack::~auto_obstack() gdb/gdb_obstack.h:101
#5 0x5ed72c in dwarf2_cu::~dwarf2_cu() gdb/dwarf2read.c:25341
#6 0x5fb5bb in std::default_delete<dwarf2_cu>::operator()(dwarf2_cu*) const \
/usr/include/c++/7/bits/unique_ptr.h:78
#7 0x5f7334 in std::unique_ptr<dwarf2_cu, \
std::default_delete<dwarf2_cu> >::~unique_ptr() \
/usr/include/c++/7/bits/unique_ptr.h:268
#8 0x5c3ce5 in init_cutu_and_read_dies gdb/dwarf2read.c:7624
#9 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
#10 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
#11 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
#12 0x612359 in read_psyms gdb/elfread.c:1311
#13 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
#14 0x867d7b in read_symbols gdb/symfile.c:821
#15 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
#16 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
#17 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
#18 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
enum_flags<symfile_add_flag>, std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
#19 0x868b64 in symbol_file_add(char const*, \
enum_flags<symfile_add_flag>, std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>) gdb/symfile.c:1217
#20 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
#21 0x868bd0 in symbol_file_add_main(char const*, \
enum_flags<symfile_add_flag>) gdb/symfile.c:1231
#22 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
#23 0x71f10e in catch_command_errors gdb/main.c:372
#24 0x71ff5f in captured_main_1 gdb/main.c:1043
#25 0x72045d in captured_main gdb/main.c:1163
#26 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
#27 0x40fd7d in main gdb/gdb.c:32
#28 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
previously allocated by thread T0 here:
#0 0x7f13804526b8 in __interceptor_malloc (/usr/lib64/libasan.so.3+0xc46b8)
#1 0x5114b5 in xmalloc gdb/common/common-utils.c:44
#2 0xa82bd5 in call_chunkfun libiberty/obstack.c:94
#3 0xa82eda in _obstack_newchunk libiberty/obstack.c:206
#4 0x477310 in allocate_on_obstack::operator new(unsigned long, obstack*) \
gdb/gdb_obstack.h:117
#5 0x5dea8c in load_partial_dies gdb/dwarf2read.c:18571
#6 0x5c487f in process_psymtab_comp_unit_reader gdb/dwarf2read.c:8054
#7 0x5c3c1f in init_cutu_and_read_dies gdb/dwarf2read.c:7689
#8 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
#9 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
#10 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
#11 0x612359 in read_psyms gdb/elfread.c:1311
#12 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
#13 0x867d7b in read_symbols gdb/symfile.c:821
#14 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
#15 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
#16 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
#17 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
enum_flags<symfile_add_flag>, \
std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
#18 0x868b64 in symbol_file_add(char const*, enum_flags<symfile_add_flag>, \
std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>) gdb/symfile.c:1217
#19 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
#20 0x868bd0 in symbol_file_add_main(char const*, \
enum_flags<symfile_add_flag>) gdb/symfile.c:1231
#21 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
#22 0x71f10e in catch_command_errors gdb/main.c:372
#23 0x71ff5f in captured_main_1 gdb/main.c:1043
#24 0x72045d in captured_main gdb/main.c:1163
#25 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
#26 0x40fd7d in main gdb/gdb.c:32
#27 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
...
This error happens as follows.
The function find_partial_die has a cu argument, but returns a pdi which may
or may not be from that cu:
...
/* Find a partial DIE at OFFSET, which may or may not be in CU,
except in the case of .debug_types DIEs which do not reference
outside their CU (they do however referencing other types via
DW_FORM_ref_sig8). */
static struct partial_die_info *
find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
...
So the pdi returned by find_partial_die here in partial_die_parent_scope may
be from another cu:
...
partial_die_parent_scope (struct partial_die_info *pdi,
struct dwarf2_cu *cu)
{
const char *grandparent_scope;
struct partial_die_info *parent, *real_pdi;
/* We need to look at our parent DIE; if we have a DW_AT_specification,
then this means the parent of the specification DIE. */
real_pdi = pdi;
while (real_pdi->has_specification)
real_pdi = find_partial_die (real_pdi->spec_offset,
real_pdi->spec_is_dwz, cu);
parent = real_pdi->die_parent;
...
in which case both real_pdi and parent will be not from cu, but from another
one, say cu2.
Subsequently, cu's comp_unit_obstack is used to set parent->scope:
...
parent->scope = typename_concat (&cu->comp_unit_obstack,
grandparent_scope,
parent->name, 0, cu);
...
So, we use cu->comp_unit_obstack to assign a value to the scope field of
a pdi belonging to cu2, and when cu is deleted, the scope field points to a
freed value.
Fix this by making find_partial_die return the cu corresponding to the
returned pdi, and handling this at the call sites.
Tested on x86_64-linux.
OK for trunk?
Thanks,
- Tom
[gdb] Fix heap-use-after-free in typename_concat
gdb/ChangeLog:
2019-05-03 Tom de Vries <tdevries@suse.de>
PR gdb/24094
* dwarf2read.c (struct cu_partial_die_info): New struct.
(find_partial_die): Return cu_partial_die_info.
(partial_die_parent_scope, guess_partial_die_structure_name)
(partial_die_info::fixup): Handle new return type of find_partial_die.
---
gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
1 file changed, 39 insertions(+), 10 deletions(-)
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index b0bdecf96f..442b618f6e 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
static struct partial_die_info *load_partial_dies
(const struct die_reader_specs *, const gdb_byte *, int);
-static struct partial_die_info *find_partial_die (sect_offset, int,
- struct dwarf2_cu *);
+struct cu_partial_die_info
+{
+ struct dwarf2_cu *cu;
+ struct partial_die_info *pdi;
+};
+
+static struct cu_partial_die_info find_partial_die (sect_offset, int,
+ struct dwarf2_cu *);
static const gdb_byte *read_attribute (const struct die_reader_specs *,
struct attribute *, struct attr_abbrev *,
@@ -8771,14 +8777,19 @@ partial_die_parent_scope (struct partial_die_info *pdi,
{
const char *grandparent_scope;
struct partial_die_info *parent, *real_pdi;
+ struct cu_partial_die_info res;
/* We need to look at our parent DIE; if we have a DW_AT_specification,
then this means the parent of the specification DIE. */
real_pdi = pdi;
while (real_pdi->has_specification)
- real_pdi = find_partial_die (real_pdi->spec_offset,
- real_pdi->spec_is_dwz, cu);
+ {
+ res = find_partial_die (real_pdi->spec_offset,
+ real_pdi->spec_is_dwz, cu);
+ real_pdi = res.pdi;
+ cu = res.cu;
+ }
parent = real_pdi->die_parent;
if (parent == NULL)
@@ -18922,7 +18933,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
outside their CU (they do however referencing other types via
DW_FORM_ref_sig8). */
-static struct partial_die_info *
+static struct cu_partial_die_info
find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
{
struct dwarf2_per_objfile *dwarf2_per_objfile
@@ -18936,7 +18947,12 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
{
pd = cu->find_partial_die (sect_off);
if (pd != NULL)
- return pd;
+ {
+ struct cu_partial_die_info res;
+ res.pdi = pd;
+ res.cu = cu;
+ return res;
+ }
/* We missed recording what we needed.
Load all dies and try again. */
per_cu = cu->per_cu;
@@ -18984,7 +19000,12 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
_("could not find partial DIE %s "
"in cache [from module %s]\n"),
sect_offset_str (sect_off), bfd_get_filename (objfile->obfd));
- return pd;
+ {
+ struct cu_partial_die_info res;
+ res.pdi = pd;
+ res.cu = per_cu->cu;
+ return res;
+ }
}
/* See if we can figure out if the class lives in a namespace. We do
@@ -19003,6 +19024,7 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
struct partial_die_info *real_pdi;
struct partial_die_info *child_pdi;
+ struct cu_partial_die_info res;
/* If this DIE (this DIE's specification, if any) has a parent, then
we should not do this. We'll prepend the parent's fully qualified
@@ -19010,8 +19032,12 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
real_pdi = struct_pdi;
while (real_pdi->has_specification)
- real_pdi = find_partial_die (real_pdi->spec_offset,
- real_pdi->spec_is_dwz, cu);
+ {
+ res = find_partial_die (real_pdi->spec_offset,
+ real_pdi->spec_is_dwz, cu);
+ real_pdi = res.pdi;
+ cu = res.cu;
+ }
if (real_pdi->die_parent != NULL)
return;
@@ -19056,8 +19082,11 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
if (name == NULL && has_specification)
{
struct partial_die_info *spec_die;
+ struct cu_partial_die_info res;
- spec_die = find_partial_die (spec_offset, spec_is_dwz, cu);
+ res = find_partial_die (spec_offset, spec_is_dwz, cu);
+ spec_die = res.pdi;
+ cu = res.cu;
spec_die->fixup (cu);
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PING][PATCH][gdb] Fix heap-use-after-free in typename_concat
2019-05-03 9:31 [PATCH][gdb] Fix heap-use-after-free in typename_concat Tom de Vries
@ 2019-05-16 8:17 ` Tom de Vries
2019-05-16 15:37 ` [PATCH][gdb] " Andrew Burgess
2019-05-16 18:53 ` Tom Tromey
2 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2019-05-16 8:17 UTC (permalink / raw)
To: gdb-patches
On 03-05-19 11:31, Tom de Vries wrote:
> Hi,
>
> When running gdb using AddressSanitizer, and loading a cc1plus binary built
> with profiledbootstrap and -flto, we run into a heap-use-after-free error:
> ...
> $ LD_PRELOAD=/usr/lib64/libasan.so.3 ./gdb -batch cc1plus
> ==26855==ERROR: AddressSanitizer: heap-use-after-free on address \
> 0x62100ad8a8b0 at pc 0x7f13803cc9e3 bp 0x7ffe55b0d090 sp 0x7ffe55b0c840
> READ of size 47 at 0x62100ad8a8b0 thread T0
> #0 0x7f13803cc9e2 (/usr/lib64/libasan.so.3+0x3e9e2)
> #1 0x5e7a0d in typename_concat gdb/dwarf2read.c:22661
> #2 0x5c6437 in partial_die_full_name gdb/dwarf2read.c:8876
> #3 0x5c6555 in add_partial_symbol gdb/dwarf2read.c:8893
> #4 0x5c6ecf in add_partial_subprogram gdb/dwarf2read.c:9156
> #5 0x5c5e90 in scan_partial_symbols gdb/dwarf2read.c:8668
> #6 0x5c6c0a in add_partial_namespace gdb/dwarf2read.c:9081
> #7 0x5c5f99 in scan_partial_symbols gdb/dwarf2read.c:8702
> #8 0x5c48b6 in process_psymtab_comp_unit_reader gdb/dwarf2read.c:8056
> #9 0x5c3c1f in init_cutu_and_read_dies gdb/dwarf2read.c:7689
> #10 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
> #11 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
> #12 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
> #13 0x612359 in read_psyms gdb/elfread.c:1311
> #14 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
> #15 0x867d7b in read_symbols gdb/symfile.c:821
> #16 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
> #17 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
> #18 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
> #19 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
> enum_flags<symfile_add_flag>, std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
> #20 0x868b64 in symbol_file_add(char const*, \
> enum_flags<symfile_add_flag>, \
> std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>) gdb/symfile.c:1217
> #21 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
> #22 0x868bd0 in symbol_file_add_main(char const*, \
> enum_flags<symfile_add_flag>) gdb/symfile.c:1231
> #23 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
> #24 0x71f10e in catch_command_errors gdb/main.c:372
> #25 0x71ff5f in captured_main_1 gdb/main.c:1043
> #26 0x72045d in captured_main gdb/main.c:1163
> #27 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
> #28 0x40fd7d in main gdb/gdb.c:32
> #29 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
> #30 0x40fc89 in _start (/data/gdb_versions/devel/build/gdb/gdb+0x40fc89)
>
> 0x62100ad8a8b0 is located 944 bytes inside of 4064-byte region \
> [0x62100ad8a500,0x62100ad8b4e0)
> freed by thread T0 here:
> #0 0x7f13804523a0 in __interceptor_free (/usr/lib64/libasan.so.3+0xc43a0)
> #1 0x435e44 in xfree<void> gdb/common/common-utils.h:60
> #2 0xa82c25 in call_freefun libiberty/obstack.c:103
> #3 0xa83098 in _obstack_free libiberty/obstack.c:280
> #4 0x4367da in auto_obstack::~auto_obstack() gdb/gdb_obstack.h:101
> #5 0x5ed72c in dwarf2_cu::~dwarf2_cu() gdb/dwarf2read.c:25341
> #6 0x5fb5bb in std::default_delete<dwarf2_cu>::operator()(dwarf2_cu*) const \
> /usr/include/c++/7/bits/unique_ptr.h:78
> #7 0x5f7334 in std::unique_ptr<dwarf2_cu, \
> std::default_delete<dwarf2_cu> >::~unique_ptr() \
> /usr/include/c++/7/bits/unique_ptr.h:268
> #8 0x5c3ce5 in init_cutu_and_read_dies gdb/dwarf2read.c:7624
> #9 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
> #10 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
> #11 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
> #12 0x612359 in read_psyms gdb/elfread.c:1311
> #13 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
> #14 0x867d7b in read_symbols gdb/symfile.c:821
> #15 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
> #16 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
> #17 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
> #18 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
> enum_flags<symfile_add_flag>, std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
> #19 0x868b64 in symbol_file_add(char const*, \
> enum_flags<symfile_add_flag>, std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>) gdb/symfile.c:1217
> #20 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
> #21 0x868bd0 in symbol_file_add_main(char const*, \
> enum_flags<symfile_add_flag>) gdb/symfile.c:1231
> #22 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
> #23 0x71f10e in catch_command_errors gdb/main.c:372
> #24 0x71ff5f in captured_main_1 gdb/main.c:1043
> #25 0x72045d in captured_main gdb/main.c:1163
> #26 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
> #27 0x40fd7d in main gdb/gdb.c:32
> #28 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
>
> previously allocated by thread T0 here:
> #0 0x7f13804526b8 in __interceptor_malloc (/usr/lib64/libasan.so.3+0xc46b8)
> #1 0x5114b5 in xmalloc gdb/common/common-utils.c:44
> #2 0xa82bd5 in call_chunkfun libiberty/obstack.c:94
> #3 0xa82eda in _obstack_newchunk libiberty/obstack.c:206
> #4 0x477310 in allocate_on_obstack::operator new(unsigned long, obstack*) \
> gdb/gdb_obstack.h:117
> #5 0x5dea8c in load_partial_dies gdb/dwarf2read.c:18571
> #6 0x5c487f in process_psymtab_comp_unit_reader gdb/dwarf2read.c:8054
> #7 0x5c3c1f in init_cutu_and_read_dies gdb/dwarf2read.c:7689
> #8 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
> #9 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
> #10 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
> #11 0x612359 in read_psyms gdb/elfread.c:1311
> #12 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
> #13 0x867d7b in read_symbols gdb/symfile.c:821
> #14 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
> #15 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
> #16 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
> #17 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
> enum_flags<symfile_add_flag>, \
> std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
> #18 0x868b64 in symbol_file_add(char const*, enum_flags<symfile_add_flag>, \
> std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>) gdb/symfile.c:1217
> #19 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
> #20 0x868bd0 in symbol_file_add_main(char const*, \
> enum_flags<symfile_add_flag>) gdb/symfile.c:1231
> #21 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
> #22 0x71f10e in catch_command_errors gdb/main.c:372
> #23 0x71ff5f in captured_main_1 gdb/main.c:1043
> #24 0x72045d in captured_main gdb/main.c:1163
> #25 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
> #26 0x40fd7d in main gdb/gdb.c:32
> #27 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
> ...
>
> This error happens as follows.
>
> The function find_partial_die has a cu argument, but returns a pdi which may
> or may not be from that cu:
> ...
> /* Find a partial DIE at OFFSET, which may or may not be in CU,
> except in the case of .debug_types DIEs which do not reference
> outside their CU (they do however referencing other types via
> DW_FORM_ref_sig8). */
>
> static struct partial_die_info *
> find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> ...
>
> So the pdi returned by find_partial_die here in partial_die_parent_scope may
> be from another cu:
> ...
> partial_die_parent_scope (struct partial_die_info *pdi,
> struct dwarf2_cu *cu)
> {
> const char *grandparent_scope;
> struct partial_die_info *parent, *real_pdi;
>
> /* We need to look at our parent DIE; if we have a DW_AT_specification,
> then this means the parent of the specification DIE. */
>
> real_pdi = pdi;
> while (real_pdi->has_specification)
> real_pdi = find_partial_die (real_pdi->spec_offset,
> real_pdi->spec_is_dwz, cu);
>
> parent = real_pdi->die_parent;
> ...
> in which case both real_pdi and parent will be not from cu, but from another
> one, say cu2.
>
> Subsequently, cu's comp_unit_obstack is used to set parent->scope:
> ...
> parent->scope = typename_concat (&cu->comp_unit_obstack,
> grandparent_scope,
> parent->name, 0, cu);
> ...
>
> So, we use cu->comp_unit_obstack to assign a value to the scope field of
> a pdi belonging to cu2, and when cu is deleted, the scope field points to a
> freed value.
>
> Fix this by making find_partial_die return the cu corresponding to the
> returned pdi, and handling this at the call sites.
>
> Tested on x86_64-linux.
>
> OK for trunk?
>
Ping.
Thanks,
- Tom
> [gdb] Fix heap-use-after-free in typename_concat
>
> gdb/ChangeLog:
>
> 2019-05-03 Tom de Vries <tdevries@suse.de>
>
> PR gdb/24094
> * dwarf2read.c (struct cu_partial_die_info): New struct.
> (find_partial_die): Return cu_partial_die_info.
> (partial_die_parent_scope, guess_partial_die_structure_name)
> (partial_die_info::fixup): Handle new return type of find_partial_die.
>
> ---
> gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index b0bdecf96f..442b618f6e 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
> static struct partial_die_info *load_partial_dies
> (const struct die_reader_specs *, const gdb_byte *, int);
>
> -static struct partial_die_info *find_partial_die (sect_offset, int,
> - struct dwarf2_cu *);
> +struct cu_partial_die_info
> +{
> + struct dwarf2_cu *cu;
> + struct partial_die_info *pdi;
> +};
> +
> +static struct cu_partial_die_info find_partial_die (sect_offset, int,
> + struct dwarf2_cu *);
>
> static const gdb_byte *read_attribute (const struct die_reader_specs *,
> struct attribute *, struct attr_abbrev *,
> @@ -8771,14 +8777,19 @@ partial_die_parent_scope (struct partial_die_info *pdi,
> {
> const char *grandparent_scope;
> struct partial_die_info *parent, *real_pdi;
> + struct cu_partial_die_info res;
>
> /* We need to look at our parent DIE; if we have a DW_AT_specification,
> then this means the parent of the specification DIE. */
>
> real_pdi = pdi;
> while (real_pdi->has_specification)
> - real_pdi = find_partial_die (real_pdi->spec_offset,
> - real_pdi->spec_is_dwz, cu);
> + {
> + res = find_partial_die (real_pdi->spec_offset,
> + real_pdi->spec_is_dwz, cu);
> + real_pdi = res.pdi;
> + cu = res.cu;
> + }
>
> parent = real_pdi->die_parent;
> if (parent == NULL)
> @@ -18922,7 +18933,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
> outside their CU (they do however referencing other types via
> DW_FORM_ref_sig8). */
>
> -static struct partial_die_info *
> +static struct cu_partial_die_info
> find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> {
> struct dwarf2_per_objfile *dwarf2_per_objfile
> @@ -18936,7 +18947,12 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> {
> pd = cu->find_partial_die (sect_off);
> if (pd != NULL)
> - return pd;
> + {
> + struct cu_partial_die_info res;
> + res.pdi = pd;
> + res.cu = cu;
> + return res;
> + }
> /* We missed recording what we needed.
> Load all dies and try again. */
> per_cu = cu->per_cu;
> @@ -18984,7 +19000,12 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> _("could not find partial DIE %s "
> "in cache [from module %s]\n"),
> sect_offset_str (sect_off), bfd_get_filename (objfile->obfd));
> - return pd;
> + {
> + struct cu_partial_die_info res;
> + res.pdi = pd;
> + res.cu = per_cu->cu;
> + return res;
> + }
> }
>
> /* See if we can figure out if the class lives in a namespace. We do
> @@ -19003,6 +19024,7 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>
> struct partial_die_info *real_pdi;
> struct partial_die_info *child_pdi;
> + struct cu_partial_die_info res;
>
> /* If this DIE (this DIE's specification, if any) has a parent, then
> we should not do this. We'll prepend the parent's fully qualified
> @@ -19010,8 +19032,12 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>
> real_pdi = struct_pdi;
> while (real_pdi->has_specification)
> - real_pdi = find_partial_die (real_pdi->spec_offset,
> - real_pdi->spec_is_dwz, cu);
> + {
> + res = find_partial_die (real_pdi->spec_offset,
> + real_pdi->spec_is_dwz, cu);
> + real_pdi = res.pdi;
> + cu = res.cu;
> + }
>
> if (real_pdi->die_parent != NULL)
> return;
> @@ -19056,8 +19082,11 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
> if (name == NULL && has_specification)
> {
> struct partial_die_info *spec_die;
> + struct cu_partial_die_info res;
>
> - spec_die = find_partial_die (spec_offset, spec_is_dwz, cu);
> + res = find_partial_die (spec_offset, spec_is_dwz, cu);
> + spec_die = res.pdi;
> + cu = res.cu;
>
> spec_die->fixup (cu);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat
2019-05-03 9:31 [PATCH][gdb] Fix heap-use-after-free in typename_concat Tom de Vries
2019-05-16 8:17 ` [PING][PATCH][gdb] " Tom de Vries
@ 2019-05-16 15:37 ` Andrew Burgess
2019-05-17 7:41 ` Tom de Vries
2019-05-16 18:53 ` Tom Tromey
2 siblings, 1 reply; 9+ messages in thread
From: Andrew Burgess @ 2019-05-16 15:37 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
* Tom de Vries <tdevries@suse.de> [2019-05-03 11:31:26 +0200]:
> Hi,
>
> When running gdb using AddressSanitizer, and loading a cc1plus binary built
> with profiledbootstrap and -flto, we run into a heap-use-after-free error:
> ...
> $ LD_PRELOAD=/usr/lib64/libasan.so.3 ./gdb -batch cc1plus
> ==26855==ERROR: AddressSanitizer: heap-use-after-free on address \
> 0x62100ad8a8b0 at pc 0x7f13803cc9e3 bp 0x7ffe55b0d090 sp 0x7ffe55b0c840
> READ of size 47 at 0x62100ad8a8b0 thread T0
> #0 0x7f13803cc9e2 (/usr/lib64/libasan.so.3+0x3e9e2)
> #1 0x5e7a0d in typename_concat gdb/dwarf2read.c:22661
> #2 0x5c6437 in partial_die_full_name gdb/dwarf2read.c:8876
> #3 0x5c6555 in add_partial_symbol gdb/dwarf2read.c:8893
> #4 0x5c6ecf in add_partial_subprogram gdb/dwarf2read.c:9156
> #5 0x5c5e90 in scan_partial_symbols gdb/dwarf2read.c:8668
> #6 0x5c6c0a in add_partial_namespace gdb/dwarf2read.c:9081
> #7 0x5c5f99 in scan_partial_symbols gdb/dwarf2read.c:8702
> #8 0x5c48b6 in process_psymtab_comp_unit_reader gdb/dwarf2read.c:8056
> #9 0x5c3c1f in init_cutu_and_read_dies gdb/dwarf2read.c:7689
> #10 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
> #11 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
> #12 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
> #13 0x612359 in read_psyms gdb/elfread.c:1311
> #14 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
> #15 0x867d7b in read_symbols gdb/symfile.c:821
> #16 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
> #17 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
> #18 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
> #19 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
> enum_flags<symfile_add_flag>, std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
> #20 0x868b64 in symbol_file_add(char const*, \
> enum_flags<symfile_add_flag>, \
> std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>) gdb/symfile.c:1217
> #21 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
> #22 0x868bd0 in symbol_file_add_main(char const*, \
> enum_flags<symfile_add_flag>) gdb/symfile.c:1231
> #23 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
> #24 0x71f10e in catch_command_errors gdb/main.c:372
> #25 0x71ff5f in captured_main_1 gdb/main.c:1043
> #26 0x72045d in captured_main gdb/main.c:1163
> #27 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
> #28 0x40fd7d in main gdb/gdb.c:32
> #29 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
> #30 0x40fc89 in _start (/data/gdb_versions/devel/build/gdb/gdb+0x40fc89)
>
> 0x62100ad8a8b0 is located 944 bytes inside of 4064-byte region \
> [0x62100ad8a500,0x62100ad8b4e0)
> freed by thread T0 here:
> #0 0x7f13804523a0 in __interceptor_free (/usr/lib64/libasan.so.3+0xc43a0)
> #1 0x435e44 in xfree<void> gdb/common/common-utils.h:60
> #2 0xa82c25 in call_freefun libiberty/obstack.c:103
> #3 0xa83098 in _obstack_free libiberty/obstack.c:280
> #4 0x4367da in auto_obstack::~auto_obstack() gdb/gdb_obstack.h:101
> #5 0x5ed72c in dwarf2_cu::~dwarf2_cu() gdb/dwarf2read.c:25341
> #6 0x5fb5bb in std::default_delete<dwarf2_cu>::operator()(dwarf2_cu*) const \
> /usr/include/c++/7/bits/unique_ptr.h:78
> #7 0x5f7334 in std::unique_ptr<dwarf2_cu, \
> std::default_delete<dwarf2_cu> >::~unique_ptr() \
> /usr/include/c++/7/bits/unique_ptr.h:268
> #8 0x5c3ce5 in init_cutu_and_read_dies gdb/dwarf2read.c:7624
> #9 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
> #10 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
> #11 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
> #12 0x612359 in read_psyms gdb/elfread.c:1311
> #13 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
> #14 0x867d7b in read_symbols gdb/symfile.c:821
> #15 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
> #16 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
> #17 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
> #18 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
> enum_flags<symfile_add_flag>, std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
> #19 0x868b64 in symbol_file_add(char const*, \
> enum_flags<symfile_add_flag>, std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>) gdb/symfile.c:1217
> #20 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
> #21 0x868bd0 in symbol_file_add_main(char const*, \
> enum_flags<symfile_add_flag>) gdb/symfile.c:1231
> #22 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
> #23 0x71f10e in catch_command_errors gdb/main.c:372
> #24 0x71ff5f in captured_main_1 gdb/main.c:1043
> #25 0x72045d in captured_main gdb/main.c:1163
> #26 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
> #27 0x40fd7d in main gdb/gdb.c:32
> #28 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
>
> previously allocated by thread T0 here:
> #0 0x7f13804526b8 in __interceptor_malloc (/usr/lib64/libasan.so.3+0xc46b8)
> #1 0x5114b5 in xmalloc gdb/common/common-utils.c:44
> #2 0xa82bd5 in call_chunkfun libiberty/obstack.c:94
> #3 0xa82eda in _obstack_newchunk libiberty/obstack.c:206
> #4 0x477310 in allocate_on_obstack::operator new(unsigned long, obstack*) \
> gdb/gdb_obstack.h:117
> #5 0x5dea8c in load_partial_dies gdb/dwarf2read.c:18571
> #6 0x5c487f in process_psymtab_comp_unit_reader gdb/dwarf2read.c:8054
> #7 0x5c3c1f in init_cutu_and_read_dies gdb/dwarf2read.c:7689
> #8 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
> #9 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
> #10 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
> #11 0x612359 in read_psyms gdb/elfread.c:1311
> #12 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
> #13 0x867d7b in read_symbols gdb/symfile.c:821
> #14 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
> #15 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
> #16 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
> #17 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
> enum_flags<symfile_add_flag>, \
> std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
> #18 0x868b64 in symbol_file_add(char const*, enum_flags<symfile_add_flag>, \
> std::vector<other_sections, \
> std::allocator<other_sections> >*, \
> enum_flags<objfile_flag>) gdb/symfile.c:1217
> #19 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
> #20 0x868bd0 in symbol_file_add_main(char const*, \
> enum_flags<symfile_add_flag>) gdb/symfile.c:1231
> #21 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
> #22 0x71f10e in catch_command_errors gdb/main.c:372
> #23 0x71ff5f in captured_main_1 gdb/main.c:1043
> #24 0x72045d in captured_main gdb/main.c:1163
> #25 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
> #26 0x40fd7d in main gdb/gdb.c:32
> #27 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
> ...
>
> This error happens as follows.
>
> The function find_partial_die has a cu argument, but returns a pdi which may
> or may not be from that cu:
> ...
> /* Find a partial DIE at OFFSET, which may or may not be in CU,
> except in the case of .debug_types DIEs which do not reference
> outside their CU (they do however referencing other types via
> DW_FORM_ref_sig8). */
>
> static struct partial_die_info *
> find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> ...
>
> So the pdi returned by find_partial_die here in partial_die_parent_scope may
> be from another cu:
> ...
> partial_die_parent_scope (struct partial_die_info *pdi,
> struct dwarf2_cu *cu)
> {
> const char *grandparent_scope;
> struct partial_die_info *parent, *real_pdi;
>
> /* We need to look at our parent DIE; if we have a DW_AT_specification,
> then this means the parent of the specification DIE. */
>
> real_pdi = pdi;
> while (real_pdi->has_specification)
> real_pdi = find_partial_die (real_pdi->spec_offset,
> real_pdi->spec_is_dwz, cu);
>
> parent = real_pdi->die_parent;
> ...
> in which case both real_pdi and parent will be not from cu, but from another
> one, say cu2.
>
> Subsequently, cu's comp_unit_obstack is used to set parent->scope:
> ...
> parent->scope = typename_concat (&cu->comp_unit_obstack,
> grandparent_scope,
> parent->name, 0, cu);
> ...
>
> So, we use cu->comp_unit_obstack to assign a value to the scope field of
> a pdi belonging to cu2, and when cu is deleted, the scope field points to a
> freed value.
>
> Fix this by making find_partial_die return the cu corresponding to the
> returned pdi, and handling this at the call sites.
>
> Tested on x86_64-linux.
>
> OK for trunk?
>
> Thanks,
> - Tom
>
> [gdb] Fix heap-use-after-free in typename_concat
>
> gdb/ChangeLog:
>
> 2019-05-03 Tom de Vries <tdevries@suse.de>
>
> PR gdb/24094
> * dwarf2read.c (struct cu_partial_die_info): New struct.
> (find_partial_die): Return cu_partial_die_info.
> (partial_die_parent_scope, guess_partial_die_structure_name)
> (partial_die_info::fixup): Handle new return type of
> find_partial_die.
This all sounds good. I have a couple of small suggestions inline
below...
>
> ---
> gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 39 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index b0bdecf96f..442b618f6e 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
> static struct partial_die_info *load_partial_dies
> (const struct die_reader_specs *, const gdb_byte *, int);
>
> -static struct partial_die_info *find_partial_die (sect_offset, int,
> - struct dwarf2_cu *);
> +struct cu_partial_die_info
> +{
> + struct dwarf2_cu *cu;
> + struct partial_die_info *pdi;
> +};
This needs at least a header comment describing its use, and ideally
each field documented too.
I wonder though if you should also provide this with a 2 argument
constructor, and delete the default constructor, like:
/* blah blah blah... */
struct cu_partial_die_info
{
/* mumble.. */
struct dwarf2_cu *cu;
/* mutter... */
struct partial_die_info *pdi;
cu_partial_die_info (struct dwarf2_cu *cu,
struct partial_die_info *pdi)
: cu (cu),
pdi (pdi)
{ /* Nothing. */ }
private:
cu_partial_die_info () = delete;
};
This will lead to some obvious knock on changes in the rest of the
code, which I think are probably improvements.
Thanks,
Andrew
> +
> +static struct cu_partial_die_info find_partial_die (sect_offset, int,
> + struct dwarf2_cu *);
>
> static const gdb_byte *read_attribute (const struct die_reader_specs *,
> struct attribute *, struct attr_abbrev *,
> @@ -8771,14 +8777,19 @@ partial_die_parent_scope (struct partial_die_info *pdi,
> {
> const char *grandparent_scope;
> struct partial_die_info *parent, *real_pdi;
> + struct cu_partial_die_info res;
>
> /* We need to look at our parent DIE; if we have a DW_AT_specification,
> then this means the parent of the specification DIE. */
>
> real_pdi = pdi;
> while (real_pdi->has_specification)
> - real_pdi = find_partial_die (real_pdi->spec_offset,
> - real_pdi->spec_is_dwz, cu);
> + {
> + res = find_partial_die (real_pdi->spec_offset,
> + real_pdi->spec_is_dwz, cu);
> + real_pdi = res.pdi;
> + cu = res.cu;
> + }
>
> parent = real_pdi->die_parent;
> if (parent == NULL)
> @@ -18922,7 +18933,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
> outside their CU (they do however referencing other types via
> DW_FORM_ref_sig8). */
>
> -static struct partial_die_info *
> +static struct cu_partial_die_info
> find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> {
> struct dwarf2_per_objfile *dwarf2_per_objfile
> @@ -18936,7 +18947,12 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> {
> pd = cu->find_partial_die (sect_off);
> if (pd != NULL)
> - return pd;
> + {
> + struct cu_partial_die_info res;
> + res.pdi = pd;
> + res.cu = cu;
> + return res;
> + }
> /* We missed recording what we needed.
> Load all dies and try again. */
> per_cu = cu->per_cu;
> @@ -18984,7 +19000,12 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> _("could not find partial DIE %s "
> "in cache [from module %s]\n"),
> sect_offset_str (sect_off), bfd_get_filename (objfile->obfd));
> - return pd;
> + {
> + struct cu_partial_die_info res;
> + res.pdi = pd;
> + res.cu = per_cu->cu;
> + return res;
> + }
> }
>
> /* See if we can figure out if the class lives in a namespace. We do
> @@ -19003,6 +19024,7 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>
> struct partial_die_info *real_pdi;
> struct partial_die_info *child_pdi;
> + struct cu_partial_die_info res;
>
> /* If this DIE (this DIE's specification, if any) has a parent, then
> we should not do this. We'll prepend the parent's fully qualified
> @@ -19010,8 +19032,12 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>
> real_pdi = struct_pdi;
> while (real_pdi->has_specification)
> - real_pdi = find_partial_die (real_pdi->spec_offset,
> - real_pdi->spec_is_dwz, cu);
> + {
> + res = find_partial_die (real_pdi->spec_offset,
> + real_pdi->spec_is_dwz, cu);
> + real_pdi = res.pdi;
> + cu = res.cu;
> + }
>
> if (real_pdi->die_parent != NULL)
> return;
> @@ -19056,8 +19082,11 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
> if (name == NULL && has_specification)
> {
> struct partial_die_info *spec_die;
> + struct cu_partial_die_info res;
>
> - spec_die = find_partial_die (spec_offset, spec_is_dwz, cu);
> + res = find_partial_die (spec_offset, spec_is_dwz, cu);
> + spec_die = res.pdi;
> + cu = res.cu;
>
> spec_die->fixup (cu);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat
2019-05-03 9:31 [PATCH][gdb] Fix heap-use-after-free in typename_concat Tom de Vries
2019-05-16 8:17 ` [PING][PATCH][gdb] " Tom de Vries
2019-05-16 15:37 ` [PATCH][gdb] " Andrew Burgess
@ 2019-05-16 18:53 ` Tom Tromey
2019-05-17 7:43 ` Tom de Vries
2 siblings, 1 reply; 9+ messages in thread
From: Tom Tromey @ 2019-05-16 18:53 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> When running gdb using AddressSanitizer, and loading a cc1plus binary built
Tom> with profiledbootstrap and -flto, we run into a heap-use-after-free error:
Thanks for finding this.
Tom> + {
Tom> + struct cu_partial_die_info res;
Tom> + res.pdi = pd;
Tom> + res.cu = cu;
Tom> + return res;
Tom> + }
Can't this be just "return {pd, cu};"?
Tom> + {
Tom> + struct cu_partial_die_info res;
Tom> + res.pdi = pd;
Tom> + res.cu = per_cu->cu;
Tom> + return res;
Tom> + }
If so, another instance of it.
thanks,
Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat
2019-05-16 15:37 ` [PATCH][gdb] " Andrew Burgess
@ 2019-05-17 7:41 ` Tom de Vries
2019-05-17 21:46 ` Andrew Burgess
0 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2019-05-17 7:41 UTC (permalink / raw)
To: Andrew Burgess, Tom Tromey; +Cc: gdb-patches
[-- Attachment #1: Type: text/plain, Size: 1922 bytes --]
On 16-05-19 17:37, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2019-05-03 11:31:26 +0200]:
> This all sounds good. I have a couple of small suggestions inline
> below...
>
>>
>> ---
>> gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 39 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>> index b0bdecf96f..442b618f6e 100644
>> --- a/gdb/dwarf2read.c
>> +++ b/gdb/dwarf2read.c
>> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
>> static struct partial_die_info *load_partial_dies
>> (const struct die_reader_specs *, const gdb_byte *, int);
>>
>> -static struct partial_die_info *find_partial_die (sect_offset, int,
>> - struct dwarf2_cu *);
>> +struct cu_partial_die_info
>> +{
>> + struct dwarf2_cu *cu;
>> + struct partial_die_info *pdi;
>> +};
>
> This needs at least a header comment describing its use, and ideally
> each field documented too.
>
Done.
> I wonder though if you should also provide this with a 2 argument
> constructor, and delete the default constructor, like:
>
> /* blah blah blah... */
>
> struct cu_partial_die_info
> {
> /* mumble.. */
> struct dwarf2_cu *cu;
>
> /* mutter... */
> struct partial_die_info *pdi;
>
> cu_partial_die_info (struct dwarf2_cu *cu,
> struct partial_die_info *pdi)
> : cu (cu),
> pdi (pdi)
> { /* Nothing. */ }
>
> private:
> cu_partial_die_info () = delete;
> };
>
> This will lead to some obvious knock on changes in the rest of the
> code, which I think are probably improvements.
>
I've tried this out, and the only effect was this type of changes:
...
- struct cu_partial_die_info res;
+ struct cu_partial_die_info res (NULL, NULL);
...
So, I've left this out for now.
Committed as below.
Also ok for 8.3 branch?
Thanks,
- Tom
[-- Attachment #2: 0001-gdb-Fix-heap-use-after-free-in-typename_concat.patch --]
[-- Type: text/x-patch, Size: 13688 bytes --]
[gdb] Fix heap-use-after-free in typename_concat
When running gdb using AddressSanitizer, and loading a cc1plus binary built
with profiledbootstrap and -flto, we run into a heap-use-after-free error:
...
$ LD_PRELOAD=/usr/lib64/libasan.so.3 ./gdb -batch cc1plus
==26855==ERROR: AddressSanitizer: heap-use-after-free on address \
0x62100ad8a8b0 at pc 0x7f13803cc9e3 bp 0x7ffe55b0d090 sp 0x7ffe55b0c840
READ of size 47 at 0x62100ad8a8b0 thread T0
#0 0x7f13803cc9e2 (/usr/lib64/libasan.so.3+0x3e9e2)
#1 0x5e7a0d in typename_concat gdb/dwarf2read.c:22661
#2 0x5c6437 in partial_die_full_name gdb/dwarf2read.c:8876
#3 0x5c6555 in add_partial_symbol gdb/dwarf2read.c:8893
#4 0x5c6ecf in add_partial_subprogram gdb/dwarf2read.c:9156
#5 0x5c5e90 in scan_partial_symbols gdb/dwarf2read.c:8668
#6 0x5c6c0a in add_partial_namespace gdb/dwarf2read.c:9081
#7 0x5c5f99 in scan_partial_symbols gdb/dwarf2read.c:8702
#8 0x5c48b6 in process_psymtab_comp_unit_reader gdb/dwarf2read.c:8056
#9 0x5c3c1f in init_cutu_and_read_dies gdb/dwarf2read.c:7689
#10 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
#11 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
#12 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
#13 0x612359 in read_psyms gdb/elfread.c:1311
#14 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
#15 0x867d7b in read_symbols gdb/symfile.c:821
#16 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
#17 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
#18 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
#19 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
enum_flags<symfile_add_flag>, std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
#20 0x868b64 in symbol_file_add(char const*, \
enum_flags<symfile_add_flag>, \
std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>) gdb/symfile.c:1217
#21 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
#22 0x868bd0 in symbol_file_add_main(char const*, \
enum_flags<symfile_add_flag>) gdb/symfile.c:1231
#23 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
#24 0x71f10e in catch_command_errors gdb/main.c:372
#25 0x71ff5f in captured_main_1 gdb/main.c:1043
#26 0x72045d in captured_main gdb/main.c:1163
#27 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
#28 0x40fd7d in main gdb/gdb.c:32
#29 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
#30 0x40fc89 in _start (/data/gdb_versions/devel/build/gdb/gdb+0x40fc89)
0x62100ad8a8b0 is located 944 bytes inside of 4064-byte region \
[0x62100ad8a500,0x62100ad8b4e0)
freed by thread T0 here:
#0 0x7f13804523a0 in __interceptor_free (/usr/lib64/libasan.so.3+0xc43a0)
#1 0x435e44 in xfree<void> gdb/common/common-utils.h:60
#2 0xa82c25 in call_freefun libiberty/obstack.c:103
#3 0xa83098 in _obstack_free libiberty/obstack.c:280
#4 0x4367da in auto_obstack::~auto_obstack() gdb/gdb_obstack.h:101
#5 0x5ed72c in dwarf2_cu::~dwarf2_cu() gdb/dwarf2read.c:25341
#6 0x5fb5bb in std::default_delete<dwarf2_cu>::operator()(dwarf2_cu*) const \
/usr/include/c++/7/bits/unique_ptr.h:78
#7 0x5f7334 in std::unique_ptr<dwarf2_cu, \
std::default_delete<dwarf2_cu> >::~unique_ptr() \
/usr/include/c++/7/bits/unique_ptr.h:268
#8 0x5c3ce5 in init_cutu_and_read_dies gdb/dwarf2read.c:7624
#9 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
#10 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
#11 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
#12 0x612359 in read_psyms gdb/elfread.c:1311
#13 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
#14 0x867d7b in read_symbols gdb/symfile.c:821
#15 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
#16 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
#17 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
#18 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
enum_flags<symfile_add_flag>, std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
#19 0x868b64 in symbol_file_add(char const*, \
enum_flags<symfile_add_flag>, std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>) gdb/symfile.c:1217
#20 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
#21 0x868bd0 in symbol_file_add_main(char const*, \
enum_flags<symfile_add_flag>) gdb/symfile.c:1231
#22 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
#23 0x71f10e in catch_command_errors gdb/main.c:372
#24 0x71ff5f in captured_main_1 gdb/main.c:1043
#25 0x72045d in captured_main gdb/main.c:1163
#26 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
#27 0x40fd7d in main gdb/gdb.c:32
#28 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
previously allocated by thread T0 here:
#0 0x7f13804526b8 in __interceptor_malloc (/usr/lib64/libasan.so.3+0xc46b8)
#1 0x5114b5 in xmalloc gdb/common/common-utils.c:44
#2 0xa82bd5 in call_chunkfun libiberty/obstack.c:94
#3 0xa82eda in _obstack_newchunk libiberty/obstack.c:206
#4 0x477310 in allocate_on_obstack::operator new(unsigned long, obstack*) \
gdb/gdb_obstack.h:117
#5 0x5dea8c in load_partial_dies gdb/dwarf2read.c:18571
#6 0x5c487f in process_psymtab_comp_unit_reader gdb/dwarf2read.c:8054
#7 0x5c3c1f in init_cutu_and_read_dies gdb/dwarf2read.c:7689
#8 0x5c4c03 in process_psymtab_comp_unit gdb/dwarf2read.c:8140
#9 0x5c58a2 in dwarf2_build_psymtabs_hard gdb/dwarf2read.c:8500
#10 0x5c0d03 in dwarf2_build_psymtabs(objfile*) gdb/dwarf2read.c:6337
#11 0x612359 in read_psyms gdb/elfread.c:1311
#12 0x798a64 in require_partial_symbols(objfile*, int) gdb/psymtab.c:115
#13 0x867d7b in read_symbols gdb/symfile.c:821
#14 0x8683d9 in syms_from_objfile_1 gdb/symfile.c:1000
#15 0x8684a1 in syms_from_objfile gdb/symfile.c:1017
#16 0x868873 in symbol_file_add_with_addrs gdb/symfile.c:1124
#17 0x868b0a in symbol_file_add_from_bfd(bfd*, char const*, \
enum_flags<symfile_add_flag>, \
std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>, objfile*) gdb/symfile.c:1204
#18 0x868b64 in symbol_file_add(char const*, enum_flags<symfile_add_flag>, \
std::vector<other_sections, \
std::allocator<other_sections> >*, \
enum_flags<objfile_flag>) gdb/symfile.c:1217
#19 0x868c39 in symbol_file_add_main_1 gdb/symfile.c:1240
#20 0x868bd0 in symbol_file_add_main(char const*, \
enum_flags<symfile_add_flag>) gdb/symfile.c:1231
#21 0x71f1b2 in symbol_file_add_main_adapter gdb/main.c:395
#22 0x71f10e in catch_command_errors gdb/main.c:372
#23 0x71ff5f in captured_main_1 gdb/main.c:1043
#24 0x72045d in captured_main gdb/main.c:1163
#25 0x7204c8 in gdb_main(captured_main_args*) gdb/main.c:1188
#26 0x40fd7d in main gdb/gdb.c:32
#27 0x7f137e300f49 in __libc_start_main (/lib64/libc.so.6+0x20f49)
...
This error happens as follows.
The function find_partial_die has a cu argument, but returns a pdi which may
or may not be from that cu:
...
/* Find a partial DIE at OFFSET, which may or may not be in CU,
except in the case of .debug_types DIEs which do not reference
outside their CU (they do however referencing other types via
DW_FORM_ref_sig8). */
static struct partial_die_info *
find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
...
So the pdi returned by find_partial_die here in partial_die_parent_scope may
be from another cu:
...
partial_die_parent_scope (struct partial_die_info *pdi,
struct dwarf2_cu *cu)
{
const char *grandparent_scope;
struct partial_die_info *parent, *real_pdi;
/* We need to look at our parent DIE; if we have a DW_AT_specification,
then this means the parent of the specification DIE. */
real_pdi = pdi;
while (real_pdi->has_specification)
real_pdi = find_partial_die (real_pdi->spec_offset,
real_pdi->spec_is_dwz, cu);
parent = real_pdi->die_parent;
...
in which case both real_pdi and parent will be not from cu, but from another
one, say cu2.
Subsequently, cu's comp_unit_obstack is used to set parent->scope:
...
parent->scope = typename_concat (&cu->comp_unit_obstack,
grandparent_scope,
parent->name, 0, cu);
...
So, we use cu->comp_unit_obstack to assign a value to the scope field of
a pdi belonging to cu2, and when cu is deleted, the scope field points to a
freed value.
Fix this by making find_partial_die return the cu corresponding to the
returned pdi, and handling this at the call sites.
Tested on x86_64-linux.
gdb/ChangeLog:
2019-05-03 Tom de Vries <tdevries@suse.de>
PR gdb/24094
* dwarf2read.c (struct cu_partial_die_info): New struct.
(find_partial_die): Return cu_partial_die_info.
(partial_die_parent_scope, guess_partial_die_structure_name)
(partial_die_info::fixup): Handle new return type of find_partial_die.
---
gdb/dwarf2read.c | 42 ++++++++++++++++++++++++++++++++----------
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 0e3f37ff74..004238a677 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1507,8 +1507,17 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
static struct partial_die_info *load_partial_dies
(const struct die_reader_specs *, const gdb_byte *, int);
-static struct partial_die_info *find_partial_die (sect_offset, int,
- struct dwarf2_cu *);
+/* A pair of partial_die_info and compilation unit. */
+struct cu_partial_die_info
+{
+ /* The compilation unit of the partial_die_info. */
+ struct dwarf2_cu *cu;
+ /* A partial_die_info. */
+ struct partial_die_info *pdi;
+};
+
+static struct cu_partial_die_info find_partial_die (sect_offset, int,
+ struct dwarf2_cu *);
static const gdb_byte *read_attribute (const struct die_reader_specs *,
struct attribute *, struct attr_abbrev *,
@@ -8754,14 +8763,19 @@ partial_die_parent_scope (struct partial_die_info *pdi,
{
const char *grandparent_scope;
struct partial_die_info *parent, *real_pdi;
+ struct cu_partial_die_info res;
/* We need to look at our parent DIE; if we have a DW_AT_specification,
then this means the parent of the specification DIE. */
real_pdi = pdi;
while (real_pdi->has_specification)
- real_pdi = find_partial_die (real_pdi->spec_offset,
- real_pdi->spec_is_dwz, cu);
+ {
+ res = find_partial_die (real_pdi->spec_offset,
+ real_pdi->spec_is_dwz, cu);
+ real_pdi = res.pdi;
+ cu = res.cu;
+ }
parent = real_pdi->die_parent;
if (parent == NULL)
@@ -18905,7 +18919,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
outside their CU (they do however referencing other types via
DW_FORM_ref_sig8). */
-static struct partial_die_info *
+static struct cu_partial_die_info
find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
{
struct dwarf2_per_objfile *dwarf2_per_objfile
@@ -18919,7 +18933,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
{
pd = cu->find_partial_die (sect_off);
if (pd != NULL)
- return pd;
+ return { cu, pd };
/* We missed recording what we needed.
Load all dies and try again. */
per_cu = cu->per_cu;
@@ -18967,7 +18981,7 @@ find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
_("could not find partial DIE %s "
"in cache [from module %s]\n"),
sect_offset_str (sect_off), bfd_get_filename (objfile->obfd));
- return pd;
+ return { per_cu->cu, pd };
}
/* See if we can figure out if the class lives in a namespace. We do
@@ -18986,6 +19000,7 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
struct partial_die_info *real_pdi;
struct partial_die_info *child_pdi;
+ struct cu_partial_die_info res;
/* If this DIE (this DIE's specification, if any) has a parent, then
we should not do this. We'll prepend the parent's fully qualified
@@ -18993,8 +19008,12 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
real_pdi = struct_pdi;
while (real_pdi->has_specification)
- real_pdi = find_partial_die (real_pdi->spec_offset,
- real_pdi->spec_is_dwz, cu);
+ {
+ res = find_partial_die (real_pdi->spec_offset,
+ real_pdi->spec_is_dwz, cu);
+ real_pdi = res.pdi;
+ cu = res.cu;
+ }
if (real_pdi->die_parent != NULL)
return;
@@ -19039,8 +19058,11 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
if (name == NULL && has_specification)
{
struct partial_die_info *spec_die;
+ struct cu_partial_die_info res;
- spec_die = find_partial_die (spec_offset, spec_is_dwz, cu);
+ res = find_partial_die (spec_offset, spec_is_dwz, cu);
+ spec_die = res.pdi;
+ cu = res.cu;
spec_die->fixup (cu);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat
2019-05-16 18:53 ` Tom Tromey
@ 2019-05-17 7:43 ` Tom de Vries
0 siblings, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2019-05-17 7:43 UTC (permalink / raw)
To: Tom Tromey; +Cc: gdb-patches
On 16-05-19 20:53, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>
> Tom> When running gdb using AddressSanitizer, and loading a cc1plus binary built
> Tom> with profiledbootstrap and -flto, we run into a heap-use-after-free error:
>
> Thanks for finding this.
>
> Tom> + {
> Tom> + struct cu_partial_die_info res;
> Tom> + res.pdi = pd;
> Tom> + res.cu = cu;
> Tom> + return res;
> Tom> + }
>
> Can't this be just "return {pd, cu};"?
>
Indeed.
Addressed at both locations in commit-post at
https://sourceware.org/ml/gdb-patches/2019-05/msg00408.html .
Thanks,
- Tom
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat
2019-05-17 7:41 ` Tom de Vries
@ 2019-05-17 21:46 ` Andrew Burgess
2019-05-18 8:51 ` Andrew Burgess
2019-05-21 13:08 ` Tom de Vries
0 siblings, 2 replies; 9+ messages in thread
From: Andrew Burgess @ 2019-05-17 21:46 UTC (permalink / raw)
To: Tom de Vries; +Cc: Tom Tromey, gdb-patches
* Tom de Vries <tdevries@suse.de> [2019-05-17 09:40:57 +0200]:
> On 16-05-19 17:37, Andrew Burgess wrote:
> > * Tom de Vries <tdevries@suse.de> [2019-05-03 11:31:26 +0200]:
>
> > This all sounds good. I have a couple of small suggestions inline
> > below...
> >
> >>
> >> ---
> >> gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
> >> 1 file changed, 39 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> >> index b0bdecf96f..442b618f6e 100644
> >> --- a/gdb/dwarf2read.c
> >> +++ b/gdb/dwarf2read.c
> >> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
> >> static struct partial_die_info *load_partial_dies
> >> (const struct die_reader_specs *, const gdb_byte *, int);
> >>
> >> -static struct partial_die_info *find_partial_die (sect_offset, int,
> >> - struct dwarf2_cu *);
> >> +struct cu_partial_die_info
> >> +{
> >> + struct dwarf2_cu *cu;
> >> + struct partial_die_info *pdi;
> >> +};
> >
> > This needs at least a header comment describing its use, and ideally
> > each field documented too.
> >
>
> Done.
>
> > I wonder though if you should also provide this with a 2 argument
> > constructor, and delete the default constructor, like:
> >
> > /* blah blah blah... */
> >
> > struct cu_partial_die_info
> > {
> > /* mumble.. */
> > struct dwarf2_cu *cu;
> >
> > /* mutter... */
> > struct partial_die_info *pdi;
> >
> > cu_partial_die_info (struct dwarf2_cu *cu,
> > struct partial_die_info *pdi)
> > : cu (cu),
> > pdi (pdi)
> > { /* Nothing. */ }
> >
> > private:
> > cu_partial_die_info () = delete;
> > };
> >
> > This will lead to some obvious knock on changes in the rest of the
> > code, which I think are probably improvements.
> >
>
> I've tried this out, and the only effect was this type of changes:
> ...
> - struct cu_partial_die_info res;
> + struct cu_partial_die_info res (NULL, NULL);
> ...
> So, I've left this out for now.
Sorry, I didn't explain myself very well.
>
> Committed as below.
I plan to apply the patch below to master (once its completed a test
run) unless you have any strong objections.
Thanks,
Andrew
---
[PATCH] gdb: Add constructor to struct cu_partial_die_info
Adds a constructor to 'struct cu_partial_die_info' and disables the
default constructor, preventing partially initialised instances from
being created.
Update 'find_partial_die' to return a const struct.
Users of 'find_partial_die' are updated to take account of the above
two changes.
There should be no user visible changes after this commit.
gdb/ChangeLog:
* dwarf2read.c (struct cu_partial_die_info): Add constructor,
delete default constructor.
(find_partial_die): Update to return const struct.
(partial_die_parent_scope): Move variable declaration into scope
of its use and change its type to auto.
(guess_partial_die_structure_name): Likewise.
(partial_die_info::fixup): Likewise.
---
gdb/ChangeLog | 10 ++++++++++
gdb/dwarf2read.c | 27 ++++++++++++++++-----------
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 004238a6775..f48b931a3f3 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -1514,10 +1514,18 @@ struct cu_partial_die_info
struct dwarf2_cu *cu;
/* A partial_die_info. */
struct partial_die_info *pdi;
+
+ cu_partial_die_info (struct dwarf2_cu *cu, struct partial_die_info *pdi)
+ : cu (cu),
+ pdi (pdi)
+ { /* Nothhing. */ }
+
+private:
+ cu_partial_die_info () = delete;
};
-static struct cu_partial_die_info find_partial_die (sect_offset, int,
- struct dwarf2_cu *);
+static const struct cu_partial_die_info find_partial_die (sect_offset, int,
+ struct dwarf2_cu *);
static const gdb_byte *read_attribute (const struct die_reader_specs *,
struct attribute *, struct attr_abbrev *,
@@ -8763,7 +8771,6 @@ partial_die_parent_scope (struct partial_die_info *pdi,
{
const char *grandparent_scope;
struct partial_die_info *parent, *real_pdi;
- struct cu_partial_die_info res;
/* We need to look at our parent DIE; if we have a DW_AT_specification,
then this means the parent of the specification DIE. */
@@ -8771,8 +8778,8 @@ partial_die_parent_scope (struct partial_die_info *pdi,
real_pdi = pdi;
while (real_pdi->has_specification)
{
- res = find_partial_die (real_pdi->spec_offset,
- real_pdi->spec_is_dwz, cu);
+ auto res = find_partial_die (real_pdi->spec_offset,
+ real_pdi->spec_is_dwz, cu);
real_pdi = res.pdi;
cu = res.cu;
}
@@ -18919,7 +18926,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
outside their CU (they do however referencing other types via
DW_FORM_ref_sig8). */
-static struct cu_partial_die_info
+static const struct cu_partial_die_info
find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
{
struct dwarf2_per_objfile *dwarf2_per_objfile
@@ -19000,7 +19007,6 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
struct partial_die_info *real_pdi;
struct partial_die_info *child_pdi;
- struct cu_partial_die_info res;
/* If this DIE (this DIE's specification, if any) has a parent, then
we should not do this. We'll prepend the parent's fully qualified
@@ -19009,8 +19015,8 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
real_pdi = struct_pdi;
while (real_pdi->has_specification)
{
- res = find_partial_die (real_pdi->spec_offset,
- real_pdi->spec_is_dwz, cu);
+ auto res = find_partial_die (real_pdi->spec_offset,
+ real_pdi->spec_is_dwz, cu);
real_pdi = res.pdi;
cu = res.cu;
}
@@ -19058,9 +19064,8 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
if (name == NULL && has_specification)
{
struct partial_die_info *spec_die;
- struct cu_partial_die_info res;
- res = find_partial_die (spec_offset, spec_is_dwz, cu);
+ auto res = find_partial_die (spec_offset, spec_is_dwz, cu);
spec_die = res.pdi;
cu = res.cu;
--
2.14.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat
2019-05-17 21:46 ` Andrew Burgess
@ 2019-05-18 8:51 ` Andrew Burgess
2019-05-21 13:08 ` Tom de Vries
1 sibling, 0 replies; 9+ messages in thread
From: Andrew Burgess @ 2019-05-18 8:51 UTC (permalink / raw)
To: Tom de Vries; +Cc: Tom Tromey, gdb-patches
* Andrew Burgess <andrew.burgess@embecosm.com> [2019-05-17 22:46:45 +0100]:
> * Tom de Vries <tdevries@suse.de> [2019-05-17 09:40:57 +0200]:
>
> > On 16-05-19 17:37, Andrew Burgess wrote:
> > > * Tom de Vries <tdevries@suse.de> [2019-05-03 11:31:26 +0200]:
> >
> > > This all sounds good. I have a couple of small suggestions inline
> > > below...
> > >
> > >>
> > >> ---
> > >> gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
> > >> 1 file changed, 39 insertions(+), 10 deletions(-)
> > >>
> > >> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> > >> index b0bdecf96f..442b618f6e 100644
> > >> --- a/gdb/dwarf2read.c
> > >> +++ b/gdb/dwarf2read.c
> > >> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
> > >> static struct partial_die_info *load_partial_dies
> > >> (const struct die_reader_specs *, const gdb_byte *, int);
> > >>
> > >> -static struct partial_die_info *find_partial_die (sect_offset, int,
> > >> - struct dwarf2_cu *);
> > >> +struct cu_partial_die_info
> > >> +{
> > >> + struct dwarf2_cu *cu;
> > >> + struct partial_die_info *pdi;
> > >> +};
> > >
> > > This needs at least a header comment describing its use, and ideally
> > > each field documented too.
> > >
> >
> > Done.
> >
> > > I wonder though if you should also provide this with a 2 argument
> > > constructor, and delete the default constructor, like:
> > >
> > > /* blah blah blah... */
> > >
> > > struct cu_partial_die_info
> > > {
> > > /* mumble.. */
> > > struct dwarf2_cu *cu;
> > >
> > > /* mutter... */
> > > struct partial_die_info *pdi;
> > >
> > > cu_partial_die_info (struct dwarf2_cu *cu,
> > > struct partial_die_info *pdi)
> > > : cu (cu),
> > > pdi (pdi)
> > > { /* Nothing. */ }
> > >
> > > private:
> > > cu_partial_die_info () = delete;
> > > };
> > >
> > > This will lead to some obvious knock on changes in the rest of the
> > > code, which I think are probably improvements.
> > >
> >
> > I've tried this out, and the only effect was this type of changes:
> > ...
> > - struct cu_partial_die_info res;
> > + struct cu_partial_die_info res (NULL, NULL);
> > ...
> > So, I've left this out for now.
>
> Sorry, I didn't explain myself very well.
>
> >
> > Committed as below.
>
> I plan to apply the patch below to master (once its completed a test
> run) unless you have any strong objections.
Passed testing, so pushed.
Thanks,
Andrew
>
> Thanks,
> Andrew
>
> ---
>
> [PATCH] gdb: Add constructor to struct cu_partial_die_info
>
> Adds a constructor to 'struct cu_partial_die_info' and disables the
> default constructor, preventing partially initialised instances from
> being created.
>
> Update 'find_partial_die' to return a const struct.
>
> Users of 'find_partial_die' are updated to take account of the above
> two changes.
>
> There should be no user visible changes after this commit.
>
> gdb/ChangeLog:
>
> * dwarf2read.c (struct cu_partial_die_info): Add constructor,
> delete default constructor.
> (find_partial_die): Update to return const struct.
> (partial_die_parent_scope): Move variable declaration into scope
> of its use and change its type to auto.
> (guess_partial_die_structure_name): Likewise.
> (partial_die_info::fixup): Likewise.
> ---
> gdb/ChangeLog | 10 ++++++++++
> gdb/dwarf2read.c | 27 ++++++++++++++++-----------
> 2 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 004238a6775..f48b931a3f3 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1514,10 +1514,18 @@ struct cu_partial_die_info
> struct dwarf2_cu *cu;
> /* A partial_die_info. */
> struct partial_die_info *pdi;
> +
> + cu_partial_die_info (struct dwarf2_cu *cu, struct partial_die_info *pdi)
> + : cu (cu),
> + pdi (pdi)
> + { /* Nothhing. */ }
> +
> +private:
> + cu_partial_die_info () = delete;
> };
>
> -static struct cu_partial_die_info find_partial_die (sect_offset, int,
> - struct dwarf2_cu *);
> +static const struct cu_partial_die_info find_partial_die (sect_offset, int,
> + struct dwarf2_cu *);
>
> static const gdb_byte *read_attribute (const struct die_reader_specs *,
> struct attribute *, struct attr_abbrev *,
> @@ -8763,7 +8771,6 @@ partial_die_parent_scope (struct partial_die_info *pdi,
> {
> const char *grandparent_scope;
> struct partial_die_info *parent, *real_pdi;
> - struct cu_partial_die_info res;
>
> /* We need to look at our parent DIE; if we have a DW_AT_specification,
> then this means the parent of the specification DIE. */
> @@ -8771,8 +8778,8 @@ partial_die_parent_scope (struct partial_die_info *pdi,
> real_pdi = pdi;
> while (real_pdi->has_specification)
> {
> - res = find_partial_die (real_pdi->spec_offset,
> - real_pdi->spec_is_dwz, cu);
> + auto res = find_partial_die (real_pdi->spec_offset,
> + real_pdi->spec_is_dwz, cu);
> real_pdi = res.pdi;
> cu = res.cu;
> }
> @@ -18919,7 +18926,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
> outside their CU (they do however referencing other types via
> DW_FORM_ref_sig8). */
>
> -static struct cu_partial_die_info
> +static const struct cu_partial_die_info
> find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> {
> struct dwarf2_per_objfile *dwarf2_per_objfile
> @@ -19000,7 +19007,6 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>
> struct partial_die_info *real_pdi;
> struct partial_die_info *child_pdi;
> - struct cu_partial_die_info res;
>
> /* If this DIE (this DIE's specification, if any) has a parent, then
> we should not do this. We'll prepend the parent's fully qualified
> @@ -19009,8 +19015,8 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
> real_pdi = struct_pdi;
> while (real_pdi->has_specification)
> {
> - res = find_partial_die (real_pdi->spec_offset,
> - real_pdi->spec_is_dwz, cu);
> + auto res = find_partial_die (real_pdi->spec_offset,
> + real_pdi->spec_is_dwz, cu);
> real_pdi = res.pdi;
> cu = res.cu;
> }
> @@ -19058,9 +19064,8 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
> if (name == NULL && has_specification)
> {
> struct partial_die_info *spec_die;
> - struct cu_partial_die_info res;
>
> - res = find_partial_die (spec_offset, spec_is_dwz, cu);
> + auto res = find_partial_die (spec_offset, spec_is_dwz, cu);
> spec_die = res.pdi;
> cu = res.cu;
>
> --
> 2.14.5
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][gdb] Fix heap-use-after-free in typename_concat
2019-05-17 21:46 ` Andrew Burgess
2019-05-18 8:51 ` Andrew Burgess
@ 2019-05-21 13:08 ` Tom de Vries
1 sibling, 0 replies; 9+ messages in thread
From: Tom de Vries @ 2019-05-21 13:08 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches
On 17-05-19 23:46, Andrew Burgess wrote:
> * Tom de Vries <tdevries@suse.de> [2019-05-17 09:40:57 +0200]:
>
>> On 16-05-19 17:37, Andrew Burgess wrote:
>>> * Tom de Vries <tdevries@suse.de> [2019-05-03 11:31:26 +0200]:
>>
>>> This all sounds good. I have a couple of small suggestions inline
>>> below...
>>>
>>>>
>>>> ---
>>>> gdb/dwarf2read.c | 49 +++++++++++++++++++++++++++++++++++++++----------
>>>> 1 file changed, 39 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
>>>> index b0bdecf96f..442b618f6e 100644
>>>> --- a/gdb/dwarf2read.c
>>>> +++ b/gdb/dwarf2read.c
>>>> @@ -1518,8 +1518,14 @@ static unsigned int peek_abbrev_code (bfd *, const gdb_byte *);
>>>> static struct partial_die_info *load_partial_dies
>>>> (const struct die_reader_specs *, const gdb_byte *, int);
>>>>
>>>> -static struct partial_die_info *find_partial_die (sect_offset, int,
>>>> - struct dwarf2_cu *);
>>>> +struct cu_partial_die_info
>>>> +{
>>>> + struct dwarf2_cu *cu;
>>>> + struct partial_die_info *pdi;
>>>> +};
>>>
>>> This needs at least a header comment describing its use, and ideally
>>> each field documented too.
>>>
>>
>> Done.
>>
>>> I wonder though if you should also provide this with a 2 argument
>>> constructor, and delete the default constructor, like:
>>>
>>> /* blah blah blah... */
>>>
>>> struct cu_partial_die_info
>>> {
>>> /* mumble.. */
>>> struct dwarf2_cu *cu;
>>>
>>> /* mutter... */
>>> struct partial_die_info *pdi;
>>>
>>> cu_partial_die_info (struct dwarf2_cu *cu,
>>> struct partial_die_info *pdi)
>>> : cu (cu),
>>> pdi (pdi)
>>> { /* Nothing. */ }
>>>
>>> private:
>>> cu_partial_die_info () = delete;
>>> };
>>>
>>> This will lead to some obvious knock on changes in the rest of the
>>> code, which I think are probably improvements.
>>>
>>
>> I've tried this out, and the only effect was this type of changes:
>> ...
>> - struct cu_partial_die_info res;
>> + struct cu_partial_die_info res (NULL, NULL);
>> ...
>> So, I've left this out for now.
>
> Sorry, I didn't explain myself very well.
>
Ah, I see what you meant now. Agreed, that's a nice cleanup.
Thanks,
- Tom
>>
>> Committed as below.
>
> I plan to apply the patch below to master (once its completed a test
> run) unless you have any strong objections.
>
> Thanks,
> Andrew
>
> ---
>
> [PATCH] gdb: Add constructor to struct cu_partial_die_info
>
> Adds a constructor to 'struct cu_partial_die_info' and disables the
> default constructor, preventing partially initialised instances from
> being created.
>
> Update 'find_partial_die' to return a const struct.
>
> Users of 'find_partial_die' are updated to take account of the above
> two changes.
>
> There should be no user visible changes after this commit.
>
> gdb/ChangeLog:
>
> * dwarf2read.c (struct cu_partial_die_info): Add constructor,
> delete default constructor.
> (find_partial_die): Update to return const struct.
> (partial_die_parent_scope): Move variable declaration into scope
> of its use and change its type to auto.
> (guess_partial_die_structure_name): Likewise.
> (partial_die_info::fixup): Likewise.
> ---
> gdb/ChangeLog | 10 ++++++++++
> gdb/dwarf2read.c | 27 ++++++++++++++++-----------
> 2 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index 004238a6775..f48b931a3f3 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -1514,10 +1514,18 @@ struct cu_partial_die_info
> struct dwarf2_cu *cu;
> /* A partial_die_info. */
> struct partial_die_info *pdi;
> +
> + cu_partial_die_info (struct dwarf2_cu *cu, struct partial_die_info *pdi)
> + : cu (cu),
> + pdi (pdi)
> + { /* Nothhing. */ }
> +
> +private:
> + cu_partial_die_info () = delete;
> };
>
> -static struct cu_partial_die_info find_partial_die (sect_offset, int,
> - struct dwarf2_cu *);
> +static const struct cu_partial_die_info find_partial_die (sect_offset, int,
> + struct dwarf2_cu *);
>
> static const gdb_byte *read_attribute (const struct die_reader_specs *,
> struct attribute *, struct attr_abbrev *,
> @@ -8763,7 +8771,6 @@ partial_die_parent_scope (struct partial_die_info *pdi,
> {
> const char *grandparent_scope;
> struct partial_die_info *parent, *real_pdi;
> - struct cu_partial_die_info res;
>
> /* We need to look at our parent DIE; if we have a DW_AT_specification,
> then this means the parent of the specification DIE. */
> @@ -8771,8 +8778,8 @@ partial_die_parent_scope (struct partial_die_info *pdi,
> real_pdi = pdi;
> while (real_pdi->has_specification)
> {
> - res = find_partial_die (real_pdi->spec_offset,
> - real_pdi->spec_is_dwz, cu);
> + auto res = find_partial_die (real_pdi->spec_offset,
> + real_pdi->spec_is_dwz, cu);
> real_pdi = res.pdi;
> cu = res.cu;
> }
> @@ -18919,7 +18926,7 @@ dwarf2_cu::find_partial_die (sect_offset sect_off)
> outside their CU (they do however referencing other types via
> DW_FORM_ref_sig8). */
>
> -static struct cu_partial_die_info
> +static const struct cu_partial_die_info
> find_partial_die (sect_offset sect_off, int offset_in_dwz, struct dwarf2_cu *cu)
> {
> struct dwarf2_per_objfile *dwarf2_per_objfile
> @@ -19000,7 +19007,6 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
>
> struct partial_die_info *real_pdi;
> struct partial_die_info *child_pdi;
> - struct cu_partial_die_info res;
>
> /* If this DIE (this DIE's specification, if any) has a parent, then
> we should not do this. We'll prepend the parent's fully qualified
> @@ -19009,8 +19015,8 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
> real_pdi = struct_pdi;
> while (real_pdi->has_specification)
> {
> - res = find_partial_die (real_pdi->spec_offset,
> - real_pdi->spec_is_dwz, cu);
> + auto res = find_partial_die (real_pdi->spec_offset,
> + real_pdi->spec_is_dwz, cu);
> real_pdi = res.pdi;
> cu = res.cu;
> }
> @@ -19058,9 +19064,8 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
> if (name == NULL && has_specification)
> {
> struct partial_die_info *spec_die;
> - struct cu_partial_die_info res;
>
> - res = find_partial_die (spec_offset, spec_is_dwz, cu);
> + auto res = find_partial_die (spec_offset, spec_is_dwz, cu);
> spec_die = res.pdi;
> cu = res.cu;
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-05-21 13:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-03 9:31 [PATCH][gdb] Fix heap-use-after-free in typename_concat Tom de Vries
2019-05-16 8:17 ` [PING][PATCH][gdb] " Tom de Vries
2019-05-16 15:37 ` [PATCH][gdb] " Andrew Burgess
2019-05-17 7:41 ` Tom de Vries
2019-05-17 21:46 ` Andrew Burgess
2019-05-18 8:51 ` Andrew Burgess
2019-05-21 13:08 ` Tom de Vries
2019-05-16 18:53 ` Tom Tromey
2019-05-17 7:43 ` Tom de Vries
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).