* [PATCH 0/3] Fix some sanitizer errors @ 2021-05-03 19:32 Tom Tromey 2021-05-03 19:32 ` [PATCH 1/3] Add virtual destructor to dwarf2_per_cu_data Tom Tromey ` (3 more replies) 0 siblings, 4 replies; 8+ messages in thread From: Tom Tromey @ 2021-05-03 19:32 UTC (permalink / raw) To: gdb-patches I tried a build with the address sanitizer, and another with the undefined behavior sanitizer. This series fixes the simplest bugs I found. There is still one remaining address sanitizer report that I didn't investigate, coming from gdb.dwarf2/dw2-icc-opaque.exp. There's also one remaining undefined behavior report, triggered by gdb.ada/access_to_packed_array.exp. Tested on x86-64 Fedora 32. Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] Add virtual destructor to dwarf2_per_cu_data 2021-05-03 19:32 [PATCH 0/3] Fix some sanitizer errors Tom Tromey @ 2021-05-03 19:32 ` Tom Tromey 2021-05-04 15:34 ` Tom Tromey 2021-05-03 19:32 ` [PATCH 2/3] Fix buffer underflow in add_path Tom Tromey ` (2 subsequent siblings) 3 siblings, 1 reply; 8+ messages in thread From: Tom Tromey @ 2021-05-03 19:32 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey Address sanitizer pointed out that the patch to use 'delete' for dwarf2_per_cu_data introduced a bug -- now it is possible to delete a signatured_type using a pointer to its base class. This patch fixes the problem by introducing a virtual destructor. gdb/ChangeLog 2021-05-03 Tom Tromey <tromey@adacore.com> * dwarf2/read.h (dwarf2_per_cu_data): Add virtual destructor. --- gdb/ChangeLog | 4 ++++ gdb/dwarf2/read.h | 2 ++ 2 files changed, 6 insertions(+) diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h index 80f0e3ee13b..f514b8316ec 100644 --- a/gdb/dwarf2/read.h +++ b/gdb/dwarf2/read.h @@ -418,6 +418,8 @@ struct dwarf2_per_cu_data { } + virtual ~dwarf2_per_cu_data () = default; + /* The start offset and length of this compilation unit. NOTE: Unlike comp_unit_head.length, this length includes initial_length_size. -- 2.26.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] Add virtual destructor to dwarf2_per_cu_data 2021-05-03 19:32 ` [PATCH 1/3] Add virtual destructor to dwarf2_per_cu_data Tom Tromey @ 2021-05-04 15:34 ` Tom Tromey 2021-05-06 17:23 ` Tom Tromey 0 siblings, 1 reply; 8+ messages in thread From: Tom Tromey @ 2021-05-04 15:34 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches >>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes: Tom> Address sanitizer pointed out that the patch to use 'delete' for Tom> dwarf2_per_cu_data introduced a bug -- now it is possible to delete a Tom> signatured_type using a pointer to its base class. This patch fixes Tom> the problem by introducing a virtual destructor. I'm thinking that instead I should add a custom deleter for these objects that knows to cast to signatured_type when appropriate. This would avoid a vtable pointer just to hold the destructor. Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] Add virtual destructor to dwarf2_per_cu_data 2021-05-04 15:34 ` Tom Tromey @ 2021-05-06 17:23 ` Tom Tromey 0 siblings, 0 replies; 8+ messages in thread From: Tom Tromey @ 2021-05-06 17:23 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches Tom> I'm thinking that instead I should add a custom deleter for these Tom> objects that knows to cast to signatured_type when appropriate. This Tom> would avoid a vtable pointer just to hold the destructor. Here's the patch. Tom commit 25e4481af0d85f3c22c824323c42bc23438c1cbf Author: Tom Tromey <tromey@adacore.com> Date: Mon May 3 12:42:58 2021 -0600 Change how dwarf2_per_cu_data is deleted Address sanitizer pointed out that the patch to use 'delete' for dwarf2_per_cu_data introduced a bug -- now it is possible to delete a signatured_type using a pointer to its base class. This patch fixes the problem by introducing a deleter and a unique_ptr specialization. A virtual destructor would be more ordinary here, but it seemed wasteful to add a vtable just for this purpose. If virtual methods are ever needed here, we can revisit this. 2021-05-06 Tom Tromey <tromey@adacore.com> * dwarf2/read.h (struct dwarf2_per_cu_data_deleter: New. (dwarf2_per_cu_data_up): New typedef. (struct dwarf2_per_bfd) <allocate_per_cu>: Change return type. <all_comp_units>: Use dwarf2_per_cu_data_up. * dwarf2/read.c (dwarf2_per_cu_data::operator()): New function. (dwarf2_per_bfd::allocate_per_cu): Return dwarf2_per_cu_data_up. (create_cu_from_index_list): Likewise. (create_signatured_type_table_from_index) (create_cus_from_debug_names_list, add_type_unit) (read_comp_units_from_section): Update. (dwarf2_find_containing_comp_unit): Change type of all_comp_units. (run_test): Update. diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 187249db47e..78a3dadbdd5 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,18 @@ +2021-05-06 Tom Tromey <tromey@adacore.com> + + * dwarf2/read.h (struct dwarf2_per_cu_data_deleter: New. + (dwarf2_per_cu_data_up): New typedef. + (struct dwarf2_per_bfd) <allocate_per_cu>: Change return type. + <all_comp_units>: Use dwarf2_per_cu_data_up. + * dwarf2/read.c (dwarf2_per_cu_data::operator()): New function. + (dwarf2_per_bfd::allocate_per_cu): Return dwarf2_per_cu_data_up. + (create_cu_from_index_list): Likewise. + (create_signatured_type_table_from_index) + (create_cus_from_debug_names_list, add_type_unit) + (read_comp_units_from_section): Update. + (dwarf2_find_containing_comp_unit): Change type of all_comp_units. + (run_test): Update. + 2021-05-06 Andrew Burgess <andrew.burgess@embecosm.com> * guile/scm-breakpoint.c (bpscm_print_breakpoint_smob): Only print diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c index 5796cf1730b..e1965c193ed 100644 --- a/gdb/dwarf2/read.c +++ b/gdb/dwarf2/read.c @@ -1760,6 +1760,17 @@ dwarf2_queue_item::~dwarf2_queue_item () } } +/* See dwarf2/read.h. */ + +void +dwarf2_per_cu_data_deleter::operator() (dwarf2_per_cu_data *data) +{ + if (data->is_debug_types) + delete static_cast<signatured_type *> (data); + else + delete data; +} + /* The return type of find_file_and_directory. Note, the enclosed string pointers are only valid while this object is valid. */ @@ -2522,10 +2533,10 @@ dw2_instantiate_symtab (dwarf2_per_cu_data *per_cu, /* See read.h. */ -std::unique_ptr<dwarf2_per_cu_data> +dwarf2_per_cu_data_up dwarf2_per_bfd::allocate_per_cu () { - std::unique_ptr<dwarf2_per_cu_data> result (new dwarf2_per_cu_data); + dwarf2_per_cu_data_up result (new dwarf2_per_cu_data); result->per_bfd = this; result->index = m_num_psymtabs++; return result; @@ -2546,13 +2557,13 @@ dwarf2_per_bfd::allocate_signatured_type () /* Return a new dwarf2_per_cu_data allocated on the per-bfd obstack, and constructed with the specified field values. */ -static std::unique_ptr<dwarf2_per_cu_data> +static dwarf2_per_cu_data_up create_cu_from_index_list (dwarf2_per_bfd *per_bfd, struct dwarf2_section_info *section, int is_dwz, sect_offset sect_off, ULONGEST length) { - std::unique_ptr<dwarf2_per_cu_data> the_cu = per_bfd->allocate_per_cu (); + dwarf2_per_cu_data_up the_cu = per_bfd->allocate_per_cu (); the_cu->sect_off = sect_off; the_cu->length = length; the_cu->section = section; @@ -2580,7 +2591,7 @@ create_cus_from_index_list (dwarf2_per_bfd *per_bfd, ULONGEST length = extract_unsigned_integer (cu_list + 8, 8, BFD_ENDIAN_LITTLE); cu_list += 2 * 8; - std::unique_ptr<dwarf2_per_cu_data> per_cu + dwarf2_per_cu_data_up per_cu = create_cu_from_index_list (per_bfd, section, is_dwz, sect_off, length); per_bfd->all_comp_units.push_back (std::move (per_cu)); @@ -2647,7 +2658,7 @@ create_signatured_type_table_from_index slot = htab_find_slot (sig_types_hash.get (), sig_type.get (), INSERT); *slot = sig_type.get (); - per_bfd->all_comp_units.push_back (std::move (sig_type)); + per_bfd->all_comp_units.emplace_back (sig_type.release ()); } per_bfd->signatured_types = std::move (sig_types_hash); @@ -2699,7 +2710,7 @@ create_signatured_type_table_from_debug_names slot = htab_find_slot (sig_types_hash.get (), sig_type.get (), INSERT); *slot = sig_type.get (); - per_objfile->per_bfd->all_comp_units.push_back (std::move (sig_type)); + per_objfile->per_bfd->all_comp_units.emplace_back (sig_type.release ()); } per_objfile->per_bfd->signatured_types = std::move (sig_types_hash); @@ -4930,7 +4941,7 @@ create_cus_from_debug_names_list (dwarf2_per_bfd *per_bfd, of the next CU as end of this CU. We create the CUs here with length 0, and in cutu_reader::cutu_reader we'll fill in the actual length. */ - std::unique_ptr<dwarf2_per_cu_data> per_cu + dwarf2_per_cu_data_up per_cu = create_cu_from_index_list (per_bfd, §ion, is_dwz, sect_off, 0); per_bfd->all_comp_units.push_back (std::move (per_cu)); @@ -4955,7 +4966,7 @@ create_cus_from_debug_names_list (dwarf2_per_bfd *per_bfd, if (i >= 1) { const ULONGEST length = sect_off_next - sect_off_prev; - std::unique_ptr<dwarf2_per_cu_data> per_cu + dwarf2_per_cu_data_up per_cu = create_cu_from_index_list (per_bfd, §ion, is_dwz, sect_off_prev, length); per_bfd->all_comp_units.push_back (std::move (per_cu)); @@ -6132,7 +6143,8 @@ add_type_unit (dwarf2_per_objfile *per_objfile, ULONGEST sig, void **slot) per_objfile->resize_symtabs (); - per_objfile->per_bfd->all_comp_units.push_back (std::move (sig_type_holder)); + per_objfile->per_bfd->all_comp_units.emplace_back + (sig_type_holder.release ()); sig_type->signature = sig; sig_type->is_debug_types = 1; if (per_objfile->per_bfd->using_index) @@ -7711,7 +7723,7 @@ read_comp_units_from_section (dwarf2_per_objfile *per_objfile, while (info_ptr < section->buffer + section->size) { - std::unique_ptr<dwarf2_per_cu_data> this_cu; + dwarf2_per_cu_data_up this_cu; sect_offset sect_off = (sect_offset) (info_ptr - section->buffer); @@ -7732,7 +7744,7 @@ read_comp_units_from_section (dwarf2_per_objfile *per_objfile, signatured_type *sig_ptr = sig_type.get (); sig_type->signature = cu_header.signature; sig_type->type_offset_in_tu = cu_header.type_cu_offset_in_tu; - this_cu = std::move (sig_type); + this_cu.reset (sig_type.release ()); void **slot = htab_find_slot (types_htab.get (), sig_ptr, INSERT); gdb_assert (slot != nullptr); @@ -24581,7 +24593,7 @@ static int dwarf2_find_containing_comp_unit (sect_offset sect_off, unsigned int offset_in_dwz, - const std::vector<std::unique_ptr<dwarf2_per_cu_data>> &all_comp_units) + const std::vector<dwarf2_per_cu_data_up> &all_comp_units) { int low, high; @@ -24647,13 +24659,13 @@ namespace find_containing_comp_unit { static void run_test () { - std::unique_ptr<dwarf2_per_cu_data> one (new dwarf2_per_cu_data); + dwarf2_per_cu_data_up one (new dwarf2_per_cu_data); dwarf2_per_cu_data *one_ptr = one.get (); - std::unique_ptr<dwarf2_per_cu_data> two (new dwarf2_per_cu_data); + dwarf2_per_cu_data_up two (new dwarf2_per_cu_data); dwarf2_per_cu_data *two_ptr = two.get (); - std::unique_ptr<dwarf2_per_cu_data> three (new dwarf2_per_cu_data); + dwarf2_per_cu_data_up three (new dwarf2_per_cu_data); dwarf2_per_cu_data *three_ptr = three.get (); - std::unique_ptr<dwarf2_per_cu_data> four (new dwarf2_per_cu_data); + dwarf2_per_cu_data_up four (new dwarf2_per_cu_data); dwarf2_per_cu_data *four_ptr = four.get (); one->length = 5; @@ -24666,7 +24678,7 @@ run_test () four->length = 7; four->is_dwz = 1; - std::vector<std::unique_ptr<dwarf2_per_cu_data>> units; + std::vector<dwarf2_per_cu_data_up> units; units.push_back (std::move (one)); units.push_back (std::move (two)); units.push_back (std::move (three)); diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h index 80f0e3ee13b..756d1934ca0 100644 --- a/gdb/dwarf2/read.h +++ b/gdb/dwarf2/read.h @@ -74,6 +74,20 @@ struct dwarf2_queue_item enum language pretend_language; }; +/* A deleter for dwarf2_per_cu_data that knows to downcast to + signatured_type as appropriate. This approach lets us avoid a + virtual destructor, which saves a bit of space. */ + +struct dwarf2_per_cu_data_deleter +{ + void operator() (dwarf2_per_cu_data *data); +}; + +/* A specialization of unique_ptr for dwarf2_per_cu_data and + subclasses. */ +typedef std::unique_ptr<dwarf2_per_cu_data, dwarf2_per_cu_data_deleter> + dwarf2_per_cu_data_up; + /* Some DWARF data can be shared across objfiles who share the same BFD, this data is stored in this object. @@ -103,7 +117,7 @@ struct dwarf2_per_bfd /* A convenience function to allocate a dwarf2_per_cu_data. The returned object has its "index" field set properly. The object is allocated on the dwarf2_per_bfd obstack. */ - std::unique_ptr<dwarf2_per_cu_data> allocate_per_cu (); + dwarf2_per_cu_data_up allocate_per_cu (); /* A convenience function to allocate a signatured_type. The returned object has its "index" field set properly. The object @@ -154,7 +168,7 @@ struct dwarf2_per_bfd /* Table of all the compilation units. This is used to locate the target compilation unit of a particular reference. */ - std::vector<std::unique_ptr<dwarf2_per_cu_data>> all_comp_units; + std::vector<dwarf2_per_cu_data_up> all_comp_units; /* Table of struct type_unit_group objects. The hash key is the DW_AT_stmt_list value. */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] Fix buffer underflow in add_path 2021-05-03 19:32 [PATCH 0/3] Fix some sanitizer errors Tom Tromey 2021-05-03 19:32 ` [PATCH 1/3] Add virtual destructor to dwarf2_per_cu_data Tom Tromey @ 2021-05-03 19:32 ` Tom Tromey 2021-05-06 14:53 ` Tom de Vries 2021-05-03 19:32 ` [PATCH 3/3] Fix ubsan build Tom Tromey 2021-05-17 18:53 ` [PATCH 0/3] Fix some sanitizer errors Tom Tromey 3 siblings, 1 reply; 8+ messages in thread From: Tom Tromey @ 2021-05-03 19:32 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey Address sanitizer pointed out a buglet in source.c:add_path. In this test, from gdb.base/source-dir.exp: (gdb) set directories :/foo:/bar ... 'p[-1]' will result in a buffer underflow. This patch fixes the bug by introducing a new check. gdb/ChangeLog 2021-05-03 Tom Tromey <tromey@adacore.com> * source.c (add_path): Check 'p' before using 'p[-1]'. --- gdb/ChangeLog | 4 ++++ gdb/source.c | 1 + 2 files changed, 5 insertions(+) diff --git a/gdb/source.c b/gdb/source.c index 6fc27ae72f7..b6dab6eb236 100644 --- a/gdb/source.c +++ b/gdb/source.c @@ -537,6 +537,7 @@ add_path (const char *dirname, char **which_path, int parse_separators) /* On MS-DOS and MS-Windows, h:\ is different from h: */ && !(p == name + 3 && name[1] == ':') /* "d:/" */ #endif + && p > name && IS_DIR_SEPARATOR (p[-1])) /* Sigh. "foo/" => "foo" */ --p; -- 2.26.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] Fix buffer underflow in add_path 2021-05-03 19:32 ` [PATCH 2/3] Fix buffer underflow in add_path Tom Tromey @ 2021-05-06 14:53 ` Tom de Vries 0 siblings, 0 replies; 8+ messages in thread From: Tom de Vries @ 2021-05-06 14:53 UTC (permalink / raw) To: Tom Tromey, gdb-patches On 5/3/21 9:32 PM, Tom Tromey wrote: > Address sanitizer pointed out a buglet in source.c:add_path. > In this test, from gdb.base/source-dir.exp: > > (gdb) set directories :/foo:/bar > > ... 'p[-1]' will result in a buffer underflow. > This patch fixes the bug by introducing a new check. > I also ran into this and came up with the same solution. LGTM. Thanks, - Tom > gdb/ChangeLog > 2021-05-03 Tom Tromey <tromey@adacore.com> > > * source.c (add_path): Check 'p' before using 'p[-1]'. > --- > gdb/ChangeLog | 4 ++++ > gdb/source.c | 1 + > 2 files changed, 5 insertions(+) > > diff --git a/gdb/source.c b/gdb/source.c > index 6fc27ae72f7..b6dab6eb236 100644 > --- a/gdb/source.c > +++ b/gdb/source.c > @@ -537,6 +537,7 @@ add_path (const char *dirname, char **which_path, int parse_separators) > /* On MS-DOS and MS-Windows, h:\ is different from h: */ > && !(p == name + 3 && name[1] == ':') /* "d:/" */ > #endif > + && p > name > && IS_DIR_SEPARATOR (p[-1])) > /* Sigh. "foo/" => "foo" */ > --p; > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] Fix ubsan build 2021-05-03 19:32 [PATCH 0/3] Fix some sanitizer errors Tom Tromey 2021-05-03 19:32 ` [PATCH 1/3] Add virtual destructor to dwarf2_per_cu_data Tom Tromey 2021-05-03 19:32 ` [PATCH 2/3] Fix buffer underflow in add_path Tom Tromey @ 2021-05-03 19:32 ` Tom Tromey 2021-05-17 18:53 ` [PATCH 0/3] Fix some sanitizer errors Tom Tromey 3 siblings, 0 replies; 8+ messages in thread From: Tom Tromey @ 2021-05-03 19:32 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey I tried a build using the undefined behavior sanitizer, and gcc gave this error: In file included from /usr/include/string.h:495, from ../gnulib/import/string.h:41, from ../../binutils-gdb/gdb/../gdbsupport/common-defs.h:95, from ../../binutils-gdb/gdb/nat/linux-osdata.c:20: In function ‘char* strncpy(char*, const char*, size_t)’, inlined from ‘void time_from_time_t(char*, int, TIME_T)’ at ../../binutils-gdb/gdb/nat/linux-osdata.c:923:15, inlined from ‘void time_from_time_t(char*, int, TIME_T)’ at ../../binutils-gdb/gdb/nat/linux-osdata.c:911:1, inlined from ‘void linux_xfer_osdata_sem(buffer*)’ at ../../binutils-gdb/gdb/nat/linux-osdata.c:1082:22: /usr/include/bits/string_fortified.h:106:34: error: ‘char* __builtin_strncpy(char*, const char*, long unsigned int)’ specified bound 32 equals destination size [-Werror=stringop-truncation] This patch fixes the problem by subtracting one from the length parameter to strncpy. I changed a couple of other similar functions -- gcc does not warn about these, but I didn't see any substantial difference between the different cases, and I think these are just latent warnings, to be triggered in the future by a change to inlining heuristics. gdb/ChangeLog 2021-05-03 Tom Tromey <tromey@adacore.com> * nat/linux-osdata.c (user_from_uid, time_from_time_t) (group_from_gid): Subtract one from strncpy length. --- gdb/ChangeLog | 5 +++++ gdb/nat/linux-osdata.c | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c index 7034dd82376..12f66d3c981 100644 --- a/gdb/nat/linux-osdata.c +++ b/gdb/nat/linux-osdata.c @@ -212,7 +212,7 @@ user_from_uid (char *user, int maxlen, uid_t uid) if (pwentry) { - strncpy (user, pwentry->pw_name, maxlen); + strncpy (user, pwentry->pw_name, maxlen - 1); /* Ensure that the user name is null-terminated. */ user[maxlen - 1] = '\0'; } @@ -920,7 +920,7 @@ time_from_time_t (char *time, int maxlen, TIME_T seconds) characters long. */ char buf[30]; const char *time_str = ctime_r (&t, buf); - strncpy (time, time_str, maxlen); + strncpy (time, time_str, maxlen - 1); time[maxlen - 1] = '\0'; } } @@ -935,7 +935,7 @@ group_from_gid (char *group, int maxlen, gid_t gid) if (grentry) { - strncpy (group, grentry->gr_name, maxlen); + strncpy (group, grentry->gr_name, maxlen - 1); /* Ensure that the group name is null-terminated. */ group[maxlen - 1] = '\0'; } -- 2.26.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] Fix some sanitizer errors 2021-05-03 19:32 [PATCH 0/3] Fix some sanitizer errors Tom Tromey ` (2 preceding siblings ...) 2021-05-03 19:32 ` [PATCH 3/3] Fix ubsan build Tom Tromey @ 2021-05-17 18:53 ` Tom Tromey 3 siblings, 0 replies; 8+ messages in thread From: Tom Tromey @ 2021-05-17 18:53 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches >>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes: Tom> I tried a build with the address sanitizer, and another with the Tom> undefined behavior sanitizer. Tom> This series fixes the simplest bugs I found. I'm checking these in now. Tom ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-05-17 18:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-03 19:32 [PATCH 0/3] Fix some sanitizer errors Tom Tromey 2021-05-03 19:32 ` [PATCH 1/3] Add virtual destructor to dwarf2_per_cu_data Tom Tromey 2021-05-04 15:34 ` Tom Tromey 2021-05-06 17:23 ` Tom Tromey 2021-05-03 19:32 ` [PATCH 2/3] Fix buffer underflow in add_path Tom Tromey 2021-05-06 14:53 ` Tom de Vries 2021-05-03 19:32 ` [PATCH 3/3] Fix ubsan build Tom Tromey 2021-05-17 18:53 ` [PATCH 0/3] Fix some sanitizer errors Tom Tromey
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).