* [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-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-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
* 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 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
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).