Re: [PATCH] Fix PR 21337 v2: segfault when re-reading symbols with remote debugging.

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-04-25 14:16, Doug Gilmore wrote:
> Sorry for the delay, here is the trace under "valgrind 
> --num-callers=111".
> 
> Note that I marked the critical call points in reread_symbols.  In
> reread_symbols at symfile.c:2631 read_symbols is called.  Via the call
> to read_symbols, bsearch_cmp is eventually called, which "reads"
> memory that was free'd memory via the call to _obstack_free that was
> called just above in reread_symbols at symfile.c:2579.
> 
> The patch moves the call to objfiles_changed to the point obstack_free
> is called, so that free'd memory will not be referenced.
> 
> Trace attached.
> 
> Thanks,
> 
> Doug
> 
> (gdb) qrun
> `/home/dgilmore/tmp/h' has changed; re-reading symbols.
> ==8380== Invalid read of size 8
> ==8380==    at 0x653949: bsearch_cmp(void const*, void const*) 
> (objfiles.c:1415)
> ==8380==    by 0x559D247: bsearch (stdlib-bsearch.h:33)
> ==8380==    by 0x653B05: find_pc_section(unsigned long) 
> (objfiles.c:1462)
> ==8380==    by 0x649D00: lookup_minimal_symbol_by_pc(unsigned long)
> (minsyms.c:785)
> ==8380==    by 0x40852B: mips_pc_is_mips(unsigned long) 
> (mips-tdep.c:1183)
> ==8380==    by 0x4086EA: mips_adjust_dwarf2_addr(unsigned long)
> (mips-tdep.c:1271)
> ==8380==    by 0x5E55C2: gdbarch_adjust_dwarf2_addr(gdbarch*, unsigned
> long) (gdbarch.c:3417)
> ==8380==    by 0x5A3EB1: read_attribute_value(die_reader_specs const*,
> attribute*, unsigned int, long, unsigned char const*)
> (dwarf2read.c:16620)
> ==8380==    by 0x5A4823: read_attribute(die_reader_specs const*,
> attribute*, attr_abbrev*, unsigned char const*) (dwarf2read.c:16846)
> ==8380==    by 0x5A13BC: read_full_die_1(die_reader_specs const*,
> die_info**, unsigned char const*, int*, int) (dwarf2read.c:15585)
> ==8380==    by 0x5A1431: read_full_die(die_reader_specs const*,
> die_info**, unsigned char const*, int*) (dwarf2read.c:15604)
> ==8380==    by 0x5890B5: init_cutu_and_read_dies(dwarf2_per_cu_data*,
> abbrev_table*, int, int, void (*)(die_reader_specs const*, unsigned
> char const*, die_info*, int, void*), void*) (dwarf2read.c:5753)
> ==8380==    by 0x58A48F:
> process_psymtab_comp_unit(dwarf2_per_cu_data*, int, language)
> (dwarf2read.c:6265)
> ==8380==    by 0x58B0D0: dwarf2_build_psymtabs_hard(objfile*)
> (dwarf2read.c:6658)
> ==8380==    by 0x5859F8: dwarf2_build_psymtabs(objfile*) 
> (dwarf2read.c:4407)
> ==8380==    by 0x498C32: read_psyms(objfile*) (elfread.c:1290)
> ==8380==    by 0x6703D8: require_partial_symbols(objfile*, int) 
> (psymtab.c:87)
> ==8380==    by 0x6ADB21: read_symbols(objfile*,
> enum_flags<symfile_add_flag>) (symfile.c:883)
> ==8380==    by 0x6B1706: reread_symbols() (symfile.c:2631)   <<<  note
> _obstack_free called at reread_symbols() (symfile.c:2579)
> ==8380==    by 0x4389A5: remote_open_1(char const*, int, target_ops*,
> int) (remote.c:5021)
> ==8380==    by 0x437AD8: remote_open(char const*, int) (remote.c:4370)
> ==8380==    by 0x6D546E: open_target(char*, int, cmd_list_element*)
> (target.c:359)
> ==8380==    by 0x4662CC: do_sfunc(cmd_list_element*, char*, int)
> (cli-decode.c:122)
> ==8380==    by 0x4692F1: cmd_func(cmd_list_element*, char*, int)
> (cli-decode.c:1887)
> ==8380==    by 0x6E76BF: execute_command(char*, int) (top.c:674)
> ==8380==    by 0x46DF03: execute_control_command(command_line*)
> (cli-script.c:494)
> ==8380==    by 0x46DD74: execute_user_command(cmd_list_element*,
> char*) (cli-script.c:423)
> ==8380==    by 0x6E7606: execute_command(char*, int) (top.c:664)
> ==8380==    by 0x5C392C: command_handler(char*) (event-top.c:590)
> ==8380==    by 0x5C3CF1: command_line_handler(char*) (event-top.c:780)
> ==8380==    by 0x5C32D2: gdb_rl_callback_handler(char*) 
> (event-top.c:213)
> ==8380==    by 0x76C1DF: rl_callback_read_char (callback.c:220)
> ==8380==    by 0x5C31EB: gdb_rl_callback_read_char_wrapper_noexcept()
> (event-top.c:175)
> ==8380==    by 0x5C3261: gdb_rl_callback_read_char_wrapper(void*)
> (event-top.c:192)
> ==8380==    by 0x5C37DB: stdin_event_handler(int, void*) 
> (event-top.c:518)
> ==8380==    by 0x5C1E60: handle_file_event(file_handler*, int)
> (event-loop.c:733)
> ==8380==    by 0x5C23EC: gdb_wait_for_event(int) (event-loop.c:859)
> ==8380==    by 0x5C1231: gdb_do_one_event() (event-loop.c:322)
> ==8380==    by 0x5C12E2: start_event_loop() (event-loop.c:371)
> ==8380==    by 0x6352DC: captured_command_loop(void*) (main.c:325)
> ==8380==    by 0x5C4CA6: catch_errors(int (*)(void*), void*, char
> const*, return_mask) (exceptions.c:236)
> ==8380==    by 0x6366C4: captured_main(void*) (main.c:1150)
> ==8380==    by 0x6366ED: gdb_main(captured_main_args*) (main.c:1160)
> ==8380==    by 0x4066C3: main (gdb.c:32)
> ==8380==  Address 0x5aaca00 is 288 bytes inside a block of size 4,064 
> free'd
> ==8380==    at 0x4C2BDEC: free (in
> /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==8380==    by 0x545CD3: xfree(void*) (common-utils.c:100)
> ==8380==    by 0x81C903: _obstack_free (obstack.c:280)
> ==8380==    by 0x6B1143: reread_symbols() (symfile.c:2579)    <<<
> ==8380==    by 0x4389A5: remote_open_1(char const*, int, target_ops*,
> int) (remote.c:5021)
> ==8380==    by 0x437AD8: remote_open(char const*, int) (remote.c:4370)
> ==8380==    by 0x6D546E: open_target(char*, int, cmd_list_element*)
> (target.c:359)
> ==8380==    by 0x4662CC: do_sfunc(cmd_list_element*, char*, int)
> (cli-decode.c:122)
> ==8380==    by 0x4692F1: cmd_func(cmd_list_element*, char*, int)
> (cli-decode.c:1887)
> ==8380==    by 0x6E76BF: execute_command(char*, int) (top.c:674)
> ==8380==    by 0x46DF03: execute_control_command(command_line*)
> (cli-script.c:494)
> ==8380==    by 0x46DD74: execute_user_command(cmd_list_element*,
> char*) (cli-script.c:423)
> ==8380==    by 0x6E7606: execute_command(char*, int) (top.c:664)
> ==8380==    by 0x5C392C: command_handler(char*) (event-top.c:590)
> ==8380==    by 0x5C3CF1: command_line_handler(char*) (event-top.c:780)
> ==8380==    by 0x5C32D2: gdb_rl_callback_handler(char*) 
> (event-top.c:213)
> ==8380==    by 0x76C1DF: rl_callback_read_char (callback.c:220)
> ==8380==    by 0x5C31EB: gdb_rl_callback_read_char_wrapper_noexcept()
> (event-top.c:175)
> ==8380==    by 0x5C3261: gdb_rl_callback_read_char_wrapper(void*)
> (event-top.c:192)
> ==8380==    by 0x5C37DB: stdin_event_handler(int, void*) 
> (event-top.c:518)
> ==8380==    by 0x5C1E60: handle_file_event(file_handler*, int)
> (event-loop.c:733)
> ==8380==    by 0x5C23EC: gdb_wait_for_event(int) (event-loop.c:859)
> ==8380==    by 0x5C1231: gdb_do_one_event() (event-loop.c:322)
> ==8380==    by 0x5C12E2: start_event_loop() (event-loop.c:371)
> ==8380==    by 0x6352DC: captured_command_loop(void*) (main.c:325)
> ==8380==    by 0x5C4CA6: catch_errors(int (*)(void*), void*, char
> const*, return_mask) (exceptions.c:236)
> ==8380==    by 0x6366C4: captured_main(void*) (main.c:1150)
> ==8380==    by 0x6366ED: gdb_main(captured_main_args*) (main.c:1160)
> ==8380==    by 0x4066C3: main (gdb.c:32)
> ==8380==

Hi Doug,

I've ran this in my head many times, but I still don't understand which 
field exactly is stale and causes the segfault.

According to the backtrace, the line of the crash is:

   if (pc < obj_section_addr (section))

That macro expands to

#define obj_section_addr(s)				      		\
   (bfd_get_section_vma ((s)->objfile->obfd, s->the_bfd_section)		\
    + obj_section_offset (s))

which further expands to

#define obj_section_offset(s)						\
   (((s)->objfile->section_offsets)->offsets[gdb_bfd_section_index 
((s)->objfile->obfd, (s)->the_bfd_section)])


Could you point out which dereferencing operator/memory access causes 
the crash?  At first I thought it would be ->section_offsets, but that 
field is set properly before calling read_symbols:

	  /* We use the same section offsets as from last time.  I'm not
	     sure whether that is always correct for shared libraries.  */
	  objfile->section_offsets = (struct section_offsets *)
	    obstack_alloc (&objfile->objfile_obstack,
			   SIZEOF_N_SECTION_OFFSETS (num_offsets));
	  memcpy (objfile->section_offsets, offsets,
		  SIZEOF_N_SECTION_OFFSETS (num_offsets));
	  objfile->num_sections = num_offsets;

so it should not be the culprit...  The s variable itself should point 
to an instance of obj_section, contained in the 
objfile_pspace_info::sections array.  This one is allocated with 
XNEWVEC, so it shouldn't be affected by the fact that we clear the 
obstack.

The objfile object itself doesn't change address, so the pointers to it 
should still be valid...

I find the obj_section_addr and obj_section_offset very bad for 
readability and debuggability.  Could you change them for inline 
functions that are not one liners?  Then it will be obvious in the 
backtrace which operation causes the crash.

Thanks,

Simon