* [PATCH 0/2] fix for the stabs debug parser @ 2022-02-19 19:51 Andrew Burgess 2022-02-19 19:51 ` [PATCH 1/2] gdb: make use of std::string in dbxread.c Andrew Burgess 2022-02-19 19:51 ` [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab Andrew Burgess 0 siblings, 2 replies; 7+ messages in thread From: Andrew Burgess @ 2022-02-19 19:51 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess Fixes bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900 Andrew Andrew Burgess (2): gdb: make use of std::string in dbxread.c gdb: avoid nullptr access in dbxread.c from read_dbx_symtab gdb/dbxread.c | 155 ++++++++++++++++++++++++++++++-------------------- 1 file changed, 92 insertions(+), 63 deletions(-) -- 2.25.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] gdb: make use of std::string in dbxread.c 2022-02-19 19:51 [PATCH 0/2] fix for the stabs debug parser Andrew Burgess @ 2022-02-19 19:51 ` Andrew Burgess 2022-02-20 1:27 ` Simon Marchi 2022-02-19 19:51 ` [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab Andrew Burgess 1 sibling, 1 reply; 7+ messages in thread From: Andrew Burgess @ 2022-02-19 19:51 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess While taking a look through dbxread.c I spotted a couple of places where making use of std::string would remove the need for manual memory allocation and memcpy. There should be no user visible changes after this commit. --- gdb/dbxread.c | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/gdb/dbxread.c b/gdb/dbxread.c index 911d518b7e9..ddda5df4ee1 100644 --- a/gdb/dbxread.c +++ b/gdb/dbxread.c @@ -1603,13 +1603,8 @@ read_dbx_symtab (minimal_symbol_reader &reader, case 'f': if (! pst) { - int name_len = p - namestring; - char *name = (char *) xmalloc (name_len + 1); - - memcpy (name, namestring, name_len); - name[name_len] = '\0'; - function_outside_compilation_unit_complaint (name); - xfree (name); + std::string name (namestring, (p - namestring)); + function_outside_compilation_unit_complaint (name.c_str ()); } /* Kludges for ELF/STABS with Sun ACC. */ last_function_name = namestring; @@ -1663,13 +1658,8 @@ read_dbx_symtab (minimal_symbol_reader &reader, case 'F': if (! pst) { - int name_len = p - namestring; - char *name = (char *) xmalloc (name_len + 1); - - memcpy (name, namestring, name_len); - name[name_len] = '\0'; - function_outside_compilation_unit_complaint (name); - xfree (name); + std::string name (namestring, (p - namestring)); + function_outside_compilation_unit_complaint (name.c_str ()); } /* Kludges for ELF/STABS with Sun ACC. */ last_function_name = namestring; -- 2.25.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] gdb: make use of std::string in dbxread.c 2022-02-19 19:51 ` [PATCH 1/2] gdb: make use of std::string in dbxread.c Andrew Burgess @ 2022-02-20 1:27 ` Simon Marchi 2022-02-21 11:50 ` Andrew Burgess 0 siblings, 1 reply; 7+ messages in thread From: Simon Marchi @ 2022-02-20 1:27 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote: > While taking a look through dbxread.c I spotted a couple of places > where making use of std::string would remove the need for manual > memory allocation and memcpy. > > There should be no user visible changes after this commit. LGTM. It's not really related, but there is the exact same code that could be changed in the exact same way in xcoffread.c, it would be nice to change it as well. Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] gdb: make use of std::string in dbxread.c 2022-02-20 1:27 ` Simon Marchi @ 2022-02-21 11:50 ` Andrew Burgess 0 siblings, 0 replies; 7+ messages in thread From: Andrew Burgess @ 2022-02-21 11:50 UTC (permalink / raw) To: Simon Marchi, gdb-patches Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote: >> While taking a look through dbxread.c I spotted a couple of places >> where making use of std::string would remove the need for manual >> memory allocation and memcpy. >> >> There should be no user visible changes after this commit. > > LGTM. > > It's not really related, but there is the exact same code that could be > changed in the exact same way in xcoffread.c, it would be nice to change > it as well. Thanks, I pushed the patch below that also fixes xcoffread.c. Andrew --- commit 9c6c44713f31f7b27bfe6921de378fa69127a048 Author: Andrew Burgess <aburgess@redhat.com> Date: Sat Feb 19 13:08:32 2022 +0000 gdb: make use of std::string in dbxread.c and xcoffread.c While taking a look through dbxread.c I spotted a couple of places where making use of std::string would remove the need for manual memory allocation and memcpy. During review Simon pointed out that the same code exists in xcoffread.c, so I've applied the same fix there too. There should be no user visible changes after this commit. diff --git a/gdb/dbxread.c b/gdb/dbxread.c index 911d518b7e9..ddda5df4ee1 100644 --- a/gdb/dbxread.c +++ b/gdb/dbxread.c @@ -1603,13 +1603,8 @@ read_dbx_symtab (minimal_symbol_reader &reader, case 'f': if (! pst) { - int name_len = p - namestring; - char *name = (char *) xmalloc (name_len + 1); - - memcpy (name, namestring, name_len); - name[name_len] = '\0'; - function_outside_compilation_unit_complaint (name); - xfree (name); + std::string name (namestring, (p - namestring)); + function_outside_compilation_unit_complaint (name.c_str ()); } /* Kludges for ELF/STABS with Sun ACC. */ last_function_name = namestring; @@ -1663,13 +1658,8 @@ read_dbx_symtab (minimal_symbol_reader &reader, case 'F': if (! pst) { - int name_len = p - namestring; - char *name = (char *) xmalloc (name_len + 1); - - memcpy (name, namestring, name_len); - name[name_len] = '\0'; - function_outside_compilation_unit_complaint (name); - xfree (name); + std::string name (namestring, (p - namestring)); + function_outside_compilation_unit_complaint (name.c_str ()); } /* Kludges for ELF/STABS with Sun ACC. */ last_function_name = namestring; diff --git a/gdb/xcoffread.c b/gdb/xcoffread.c index d3e9ade72cb..f6db7f9ff7e 100644 --- a/gdb/xcoffread.c +++ b/gdb/xcoffread.c @@ -2734,13 +2734,8 @@ scan_xcoff_symtab (minimal_symbol_reader &reader, case 'f': if (! pst) { - int name_len = p - namestring; - char *name = (char *) xmalloc (name_len + 1); - - memcpy (name, namestring, name_len); - name[name_len] = '\0'; - function_outside_compilation_unit_complaint (name); - xfree (name); + std::string name (namestring, (p - namestring)); + function_outside_compilation_unit_complaint (name.c_str ()); } pst->add_psymbol (gdb::string_view (namestring, p - namestring), @@ -2758,13 +2753,8 @@ scan_xcoff_symtab (minimal_symbol_reader &reader, case 'F': if (! pst) { - int name_len = p - namestring; - char *name = (char *) xmalloc (name_len + 1); - - memcpy (name, namestring, name_len); - name[name_len] = '\0'; - function_outside_compilation_unit_complaint (name); - xfree (name); + std::string name (namestring, (p - namestring)); + function_outside_compilation_unit_complaint (name.c_str ()); } /* We need only the minimal symbols for these ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab 2022-02-19 19:51 [PATCH 0/2] fix for the stabs debug parser Andrew Burgess 2022-02-19 19:51 ` [PATCH 1/2] gdb: make use of std::string in dbxread.c Andrew Burgess @ 2022-02-19 19:51 ` Andrew Burgess 2022-02-20 1:30 ` Simon Marchi 1 sibling, 1 reply; 7+ messages in thread From: Andrew Burgess @ 2022-02-19 19:51 UTC (permalink / raw) To: gdb-patches; +Cc: Andrew Burgess This fixes a GDB crash reported in bug pr/28900, related to reading in some stabs debug information. In this commit my goal is to stop GDB crashing. I am not trying to ensure that GDB makes the best possible use of the available stabs debug information. At this point I consider stabs a legacy debug format, with only limited support in GDB. So, the problem appears to be that, when reading in the stabs data, we need to find a N_SO entry, this is the entry that defines the start of a compilation unit (or at least the location of a corresponding source file). It is while handling an N_SO that GDB creates a psymtab to hold the incoming debug information (symbols, etc). The problem we hit in the bug is that we encounter some symbol information (an N_PC entry) outside of an N_SO entry - that is we find some symbol information that is not associated with any source file. We already have some protection for this case, look (in read_dbx_symtab) at the handling of N_PC entries of type 'F' and 'f', if we have no psymtab (the pst variable is nullptr) then we issue a complaint. However, for whatever reason, in both 'f' and 'F' handling, there is one place where we assume that the pst variable (the psymtab) is not nullptr. This is a mistake. In this commit, I guard these two locations (in 'f' and 'F' handling) so we no longer assume pst is not nullptr. While I was at it, I audited all the other uses of pst in read_dbx_symtab, and in every potentially dangerous case I added a nullptr check, and issue a suitable complaint if pst is found to be nullptr. It might well be true that we could/should do something smarter if we see a debug symbol outside of an N_SO entry, and if anyone wanted to do that work, they're welcome too. But this commit is just about preventing the nullptr access, and the subsequent GDB crash. I don't have any tests for this change, I have no idea how to generate weird stabs data for testing. The original binary from the bug report now loads just fine without GDB crashing. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900 --- gdb/dbxread.c | 137 ++++++++++++++++++++++++++++++++------------------ 1 file changed, 88 insertions(+), 49 deletions(-) diff --git a/gdb/dbxread.c b/gdb/dbxread.c index ddda5df4ee1..165040dd7ce 100644 --- a/gdb/dbxread.c +++ b/gdb/dbxread.c @@ -1461,23 +1461,33 @@ read_dbx_symtab (minimal_symbol_reader &reader, switch (p[1]) { case 'S': - pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, - VAR_DOMAIN, LOC_STATIC, - data_sect_index, - psymbol_placement::STATIC, - nlist.n_value, psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, + VAR_DOMAIN, LOC_STATIC, + data_sect_index, + psymbol_placement::STATIC, + nlist.n_value, psymtab_language, + partial_symtabs, objfile); + else + complaint (_("static `%*s' appears to be defined " + "outside of all compilation units"), + sym_len, sym_name); continue; case 'G': /* The addresses in these entries are reported to be wrong. See the code that reads 'G's for symtabs. */ - pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, - VAR_DOMAIN, LOC_STATIC, - data_sect_index, - psymbol_placement::GLOBAL, - nlist.n_value, psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, + VAR_DOMAIN, LOC_STATIC, + data_sect_index, + psymbol_placement::GLOBAL, + nlist.n_value, psymtab_language, + partial_symtabs, objfile); + else + complaint (_("global `%*s' appears to be defined " + "outside of all compilation units"), + sym_len, sym_name); continue; case 'T': @@ -1491,19 +1501,30 @@ read_dbx_symtab (minimal_symbol_reader &reader, || (p == namestring + 1 && namestring[0] != ' ')) { - pst->add_psymbol (gdb::string_view (sym_name, sym_len), - true, STRUCT_DOMAIN, LOC_TYPEDEF, -1, - psymbol_placement::STATIC, - 0, psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (sym_name, sym_len), + true, STRUCT_DOMAIN, LOC_TYPEDEF, -1, + psymbol_placement::STATIC, + 0, psymtab_language, + partial_symtabs, objfile); + else + complaint (_("enum, struct, or union `%*s' appears " + "to be defined outside of all " + "compilation units"), + sym_len, sym_name); if (p[2] == 't') { /* Also a typedef with the same name. */ - pst->add_psymbol (gdb::string_view (sym_name, sym_len), - true, VAR_DOMAIN, LOC_TYPEDEF, -1, - psymbol_placement::STATIC, - 0, psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (sym_name, sym_len), + true, VAR_DOMAIN, LOC_TYPEDEF, -1, + psymbol_placement::STATIC, + 0, psymtab_language, + partial_symtabs, objfile); + else + complaint (_("typedef `%*s' appears to be defined " + "outside of all compilation units"), + sym_len, sym_name); p += 1; } } @@ -1512,11 +1533,16 @@ read_dbx_symtab (minimal_symbol_reader &reader, case 't': if (p != namestring) /* a name is there, not just :T... */ { - pst->add_psymbol (gdb::string_view (sym_name, sym_len), - true, VAR_DOMAIN, LOC_TYPEDEF, -1, - psymbol_placement::STATIC, - 0, psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (sym_name, sym_len), + true, VAR_DOMAIN, LOC_TYPEDEF, -1, + psymbol_placement::STATIC, + 0, psymtab_language, + partial_symtabs, objfile); + else + complaint (_("typename `%*s' appears to be defined " + "outside of all compilation units"), + sym_len, sym_name); } check_enum: /* If this is an enumerated type, we need to @@ -1574,11 +1600,16 @@ read_dbx_symtab (minimal_symbol_reader &reader, ; /* Note that the value doesn't matter for enum constants in psymtabs, just in symtabs. */ - pst->add_psymbol (gdb::string_view (p, q - p), true, - VAR_DOMAIN, LOC_CONST, -1, - psymbol_placement::STATIC, 0, - psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (p, q - p), true, + VAR_DOMAIN, LOC_CONST, -1, + psymbol_placement::STATIC, 0, + psymtab_language, + partial_symtabs, objfile); + else + complaint (_("enum constant `%*s' appears to be defined " + "outside of all compilation units"), + ((int) (q - p)), p); /* Point past the name. */ p = q; /* Skip over the value. */ @@ -1593,11 +1624,17 @@ read_dbx_symtab (minimal_symbol_reader &reader, case 'c': /* Constant, e.g. from "const" in Pascal. */ - pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, - VAR_DOMAIN, LOC_CONST, -1, - psymbol_placement::STATIC, 0, - psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, + VAR_DOMAIN, LOC_CONST, -1, + psymbol_placement::STATIC, 0, + psymtab_language, + partial_symtabs, objfile); + else + complaint (_("constant `%*s' appears to be defined " + "outside of all compilation units"), + sym_len, sym_name); + continue; case 'f': @@ -1644,12 +1681,13 @@ read_dbx_symtab (minimal_symbol_reader &reader, pst->set_text_low (nlist.n_value); textlow_not_set = 0; } - pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, - VAR_DOMAIN, LOC_BLOCK, - SECT_OFF_TEXT (objfile), - psymbol_placement::STATIC, - nlist.n_value, psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, + VAR_DOMAIN, LOC_BLOCK, + SECT_OFF_TEXT (objfile), + psymbol_placement::STATIC, + nlist.n_value, psymtab_language, + partial_symtabs, objfile); continue; /* Global functions were ignored here, but now they @@ -1699,12 +1737,13 @@ read_dbx_symtab (minimal_symbol_reader &reader, pst->set_text_low (nlist.n_value); textlow_not_set = 0; } - pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, - VAR_DOMAIN, LOC_BLOCK, - SECT_OFF_TEXT (objfile), - psymbol_placement::GLOBAL, - nlist.n_value, psymtab_language, - partial_symtabs, objfile); + if (pst != nullptr) + pst->add_psymbol (gdb::string_view (sym_name, sym_len), true, + VAR_DOMAIN, LOC_BLOCK, + SECT_OFF_TEXT (objfile), + psymbol_placement::GLOBAL, + nlist.n_value, psymtab_language, + partial_symtabs, objfile); continue; /* Two things show up here (hopefully); static symbols of -- 2.25.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab 2022-02-19 19:51 ` [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab Andrew Burgess @ 2022-02-20 1:30 ` Simon Marchi 2022-02-21 11:51 ` Andrew Burgess 0 siblings, 1 reply; 7+ messages in thread From: Simon Marchi @ 2022-02-20 1:30 UTC (permalink / raw) To: Andrew Burgess, gdb-patches On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote: > This fixes a GDB crash reported in bug pr/28900, related to reading in > some stabs debug information. > > In this commit my goal is to stop GDB crashing. I am not trying to > ensure that GDB makes the best possible use of the available stabs > debug information. At this point I consider stabs a legacy debug > format, with only limited support in GDB. > > So, the problem appears to be that, when reading in the stabs data, we > need to find a N_SO entry, this is the entry that defines the start of > a compilation unit (or at least the location of a corresponding source > file). > > It is while handling an N_SO that GDB creates a psymtab to hold the > incoming debug information (symbols, etc). > > The problem we hit in the bug is that we encounter some symbol > information (an N_PC entry) outside of an N_SO entry - that is we find > some symbol information that is not associated with any source file. > > We already have some protection for this case, look (in > read_dbx_symtab) at the handling of N_PC entries of type 'F' and 'f', > if we have no psymtab (the pst variable is nullptr) then we issue a > complaint. However, for whatever reason, in both 'f' and 'F' > handling, there is one place where we assume that the pst > variable (the psymtab) is not nullptr. This is a mistake. > > In this commit, I guard these two locations (in 'f' and 'F' handling) > so we no longer assume pst is not nullptr. > > While I was at it, I audited all the other uses of pst in > read_dbx_symtab, and in every potentially dangerous case I added a > nullptr check, and issue a suitable complaint if pst is found to be > nullptr. > > It might well be true that we could/should do something smarter if we > see a debug symbol outside of an N_SO entry, and if anyone wanted to > do that work, they're welcome too. But this commit is just about > preventing the nullptr access, and the subsequent GDB crash. > > I don't have any tests for this change, I have no idea how to generate > weird stabs data for testing. The original binary from the bug report > now loads just fine without GDB crashing. > > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900 For all I know, this looks correct. Thanks for doing this. Simon ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab 2022-02-20 1:30 ` Simon Marchi @ 2022-02-21 11:51 ` Andrew Burgess 0 siblings, 0 replies; 7+ messages in thread From: Andrew Burgess @ 2022-02-21 11:51 UTC (permalink / raw) To: Simon Marchi, gdb-patches Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes: > On 2022-02-19 14:51, Andrew Burgess via Gdb-patches wrote: >> This fixes a GDB crash reported in bug pr/28900, related to reading in >> some stabs debug information. >> >> In this commit my goal is to stop GDB crashing. I am not trying to >> ensure that GDB makes the best possible use of the available stabs >> debug information. At this point I consider stabs a legacy debug >> format, with only limited support in GDB. >> >> So, the problem appears to be that, when reading in the stabs data, we >> need to find a N_SO entry, this is the entry that defines the start of >> a compilation unit (or at least the location of a corresponding source >> file). >> >> It is while handling an N_SO that GDB creates a psymtab to hold the >> incoming debug information (symbols, etc). >> >> The problem we hit in the bug is that we encounter some symbol >> information (an N_PC entry) outside of an N_SO entry - that is we find >> some symbol information that is not associated with any source file. >> >> We already have some protection for this case, look (in >> read_dbx_symtab) at the handling of N_PC entries of type 'F' and 'f', >> if we have no psymtab (the pst variable is nullptr) then we issue a >> complaint. However, for whatever reason, in both 'f' and 'F' >> handling, there is one place where we assume that the pst >> variable (the psymtab) is not nullptr. This is a mistake. >> >> In this commit, I guard these two locations (in 'f' and 'F' handling) >> so we no longer assume pst is not nullptr. >> >> While I was at it, I audited all the other uses of pst in >> read_dbx_symtab, and in every potentially dangerous case I added a >> nullptr check, and issue a suitable complaint if pst is found to be >> nullptr. >> >> It might well be true that we could/should do something smarter if we >> see a debug symbol outside of an N_SO entry, and if anyone wanted to >> do that work, they're welcome too. But this commit is just about >> preventing the nullptr access, and the subsequent GDB crash. >> >> I don't have any tests for this change, I have no idea how to generate >> weird stabs data for testing. The original binary from the bug report >> now loads just fine without GDB crashing. >> >> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28900 > > For all I know, this looks correct. Thanks for doing this. Thanks, I pushed this patch. Andrew ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-02-21 11:51 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-02-19 19:51 [PATCH 0/2] fix for the stabs debug parser Andrew Burgess 2022-02-19 19:51 ` [PATCH 1/2] gdb: make use of std::string in dbxread.c Andrew Burgess 2022-02-20 1:27 ` Simon Marchi 2022-02-21 11:50 ` Andrew Burgess 2022-02-19 19:51 ` [PATCH 2/2] gdb: avoid nullptr access in dbxread.c from read_dbx_symtab Andrew Burgess 2022-02-20 1:30 ` Simon Marchi 2022-02-21 11:51 ` Andrew Burgess
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).