From 67aa1f3c2881e607081d9e1b57be3e7544c2c45c Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Thu, 10 Jan 2019 17:52:39 +0000 Subject: [PATCH] Fix tracepoint.c:parse_tracepoint_definition leak (and one more) Coverity points out that gdb/tracepoint.c:parse_tracepoint_definition can leak 'cond' in this line: cond = (char *) xmalloc (2 * xlen + 1); That can leak because we're in a loop and 'cond' may have already been xmalloc'ed into in a previous iteration. That won't normally happen, because we don't expect to see a tracepoint definition with multiple conditions listed, but, it doesn't hurt to be pedantically correct, in case some stub manages to send something odd back to GDB. At first I thought I'd just replace the xmalloc call with: cond = (char *) xrealloc (cond, 2 * xlen + 1); and be done with it. However, my pedantic self realizes that warning() can throw as well (due to pagination + Ctrl-C), so I fixed it using gdb::unique_xmalloc_ptr instead. While doing this, I noticed that these vectors in struct uploaded_tp: std::vector actions; std::vector step_actions; hold heap-allocated strings, but nothing is freeing the strings, AFAICS. So I ended up switching all the heap-allocated strings in uploaded_tp to unique pointers. This patch is the result of that. I also wrote an alternative, but similar patch that uses std::string throughout instead of gdb::unique_xmalloc_ptr, but in the end reverted it because the code didn't look that much better, and I kind of dislike replacing pointers with fat std::string's (3 or 4 times the size of a pointer) in structures. gdb/ChangeLog: 2019-01-10 Pedro Alves * breakpoint.c (read_uploaded_action) (create_tracepoint_from_upload): Adjust to use gdb::unique_xmalloc_ptr. * ctf.c (ctf_write_uploaded_tp): (SET_ARRAY_FIELD): Use emplace_back. (SET_STRING_FIELD): Adjust to use gdb::unique_xmalloc_ptr. * tracefile-tfile.c (tfile_write_uploaded_tp): * tracepoint.c (parse_tracepoint_definition): Adjust to use gdb::unique_xmalloc_ptr. * tracepoint.h (struct uploaded_tp) : Replace char pointers with gdb::unique_xmalloc_ptr. --- gdb/ChangeLog | 15 +++++++++++++++ gdb/breakpoint.c | 6 +++--- gdb/ctf.c | 30 +++++++++++++++++------------- gdb/tracefile-tfile.c | 21 +++++++++++---------- gdb/tracepoint.c | 24 +++++++++++++----------- gdb/tracepoint.h | 12 ++++++------ 6 files changed, 65 insertions(+), 43 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 1c036bb..b702acd 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,20 @@ 2019-01-10 Pedro Alves + * breakpoint.c (read_uploaded_action) + (create_tracepoint_from_upload): Adjust to use + gdb::unique_xmalloc_ptr. + * ctf.c (ctf_write_uploaded_tp): + (SET_ARRAY_FIELD): Use emplace_back. + (SET_STRING_FIELD): Adjust to use gdb::unique_xmalloc_ptr. + * tracefile-tfile.c (tfile_write_uploaded_tp): + * tracepoint.c (parse_tracepoint_definition): Adjust to use + gdb::unique_xmalloc_ptr. + * tracepoint.h (struct uploaded_tp) : Replace char pointers + with gdb::unique_xmalloc_ptr. + +2019-01-10 Pedro Alves + * solib-target.c (library_list_start_library): Don't xstrdup name. 2019-01-10 Pedro Alves diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 48a1c64..2ab8a76 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -14686,7 +14686,7 @@ read_uploaded_action (void) if (next_cmd < this_utp->cmd_strings.size ()) { - rslt = this_utp->cmd_strings[next_cmd]; + rslt = this_utp->cmd_strings[next_cmd].get (); next_cmd++; } @@ -14707,7 +14707,7 @@ create_tracepoint_from_upload (struct uploaded_tp *utp) struct tracepoint *tp; if (utp->at_string) - addr_str = utp->at_string; + addr_str = utp->at_string.get (); else { /* In the absence of a source location, fall back to raw @@ -14731,7 +14731,7 @@ create_tracepoint_from_upload (struct uploaded_tp *utp) current_language); if (!create_breakpoint (get_current_arch (), location.get (), - utp->cond_string, -1, addr_str, + utp->cond_string.get (), -1, addr_str, 0 /* parse cond/thread */, 0 /* tempflag */, utp->type /* type_wanted */, diff --git a/gdb/ctf.c b/gdb/ctf.c index d99e1cd..7a95df7 100644 --- a/gdb/ctf.c +++ b/gdb/ctf.c @@ -596,38 +596,42 @@ ctf_write_uploaded_tp (struct trace_file_writer *self, /* condition */ if (tp->cond != NULL) - ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond, strlen (tp->cond)); + ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond.get (), + strlen (tp->cond.get ())); ctf_save_write (&writer->tcs, &zero, 1); /* actions */ u32 = tp->actions.size (); ctf_save_align_write (&writer->tcs, (gdb_byte *) &u32, 4, 4); - for (char *act : tp->actions) - ctf_save_write (&writer->tcs, (gdb_byte *) act, strlen (act) + 1); + for (const auto &act : tp->actions) + ctf_save_write (&writer->tcs, (gdb_byte *) act.get (), + strlen (act.get ()) + 1); /* step_actions */ u32 = tp->step_actions.size (); ctf_save_align_write (&writer->tcs, (gdb_byte *) &u32, 4, 4); - for (char *act : tp->step_actions) - ctf_save_write (&writer->tcs, (gdb_byte *) act, strlen (act) + 1); + for (const auto &act : tp->step_actions) + ctf_save_write (&writer->tcs, (gdb_byte *) act.get (), + strlen (act.get ()) + 1); /* at_string */ if (tp->at_string != NULL) - ctf_save_write (&writer->tcs, (gdb_byte *) tp->at_string, - strlen (tp->at_string)); + ctf_save_write (&writer->tcs, (gdb_byte *) tp->at_string.get (), + strlen (tp->at_string.get ())); ctf_save_write (&writer->tcs, &zero, 1); /* cond_string */ if (tp->cond_string != NULL) - ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond_string, - strlen (tp->cond_string)); + ctf_save_write (&writer->tcs, (gdb_byte *) tp->cond_string.get (), + strlen (tp->cond_string.get ())); ctf_save_write (&writer->tcs, &zero, 1); /* cmd_strings */ u32 = tp->cmd_strings.size (); ctf_save_align_write (&writer->tcs, (gdb_byte *) &u32, 4, 4); - for (char *act : tp->cmd_strings) - ctf_save_write (&writer->tcs, (gdb_byte *) act, strlen (act) + 1); + for (const auto &act : tp->cmd_strings) + ctf_save_write (&writer->tcs, (gdb_byte *) act.get (), + strlen (act.get ()) + 1); } @@ -1023,7 +1027,7 @@ ctf_read_tsv (struct uploaded_tsv **uploaded_tsvs) const struct bt_definition *element \ = bt_ctf_get_index ((EVENT), def, i); \ \ - (VAR)->ARRAY.push_back \ + (VAR)->ARRAY.emplace_back \ (xstrdup (bt_ctf_get_string (element))); \ } \ } \ @@ -1040,7 +1044,7 @@ ctf_read_tsv (struct uploaded_tsv **uploaded_tsvs) #FIELD)); \ \ if (strlen (p) > 0) \ - (VAR)->FIELD = xstrdup (p); \ + (VAR)->FIELD.reset (xstrdup (p)); \ else \ (VAR)->FIELD = NULL; \ } \ diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c index afff422..831f162 100644 --- a/gdb/tracefile-tfile.c +++ b/gdb/tracefile-tfile.c @@ -262,31 +262,32 @@ tfile_write_uploaded_tp (struct trace_file_writer *self, fprintf (writer->fp, ":F%x", utp->orig_size); if (utp->cond) fprintf (writer->fp, - ":X%x,%s", (unsigned int) strlen (utp->cond) / 2, - utp->cond); + ":X%x,%s", (unsigned int) strlen (utp->cond.get ()) / 2, + utp->cond.get ()); fprintf (writer->fp, "\n"); - for (char *act : utp->actions) + for (const auto &act : utp->actions) fprintf (writer->fp, "tp A%x:%s:%s\n", - utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act); - for (char *act : utp->step_actions) + utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act.get ()); + for (const auto &act : utp->step_actions) fprintf (writer->fp, "tp S%x:%s:%s\n", - utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act); + utp->number, phex_nz (utp->addr, sizeof (utp->addr)), act.get ()); if (utp->at_string) { encode_source_string (utp->number, utp->addr, - "at", utp->at_string, buf, MAX_TRACE_UPLOAD); + "at", utp->at_string.get (), + buf, MAX_TRACE_UPLOAD); fprintf (writer->fp, "tp Z%s\n", buf); } if (utp->cond_string) { encode_source_string (utp->number, utp->addr, - "cond", utp->cond_string, + "cond", utp->cond_string.get (), buf, MAX_TRACE_UPLOAD); fprintf (writer->fp, "tp Z%s\n", buf); } - for (char *act : utp->cmd_strings) + for (const auto &act : utp->cmd_strings) { - encode_source_string (utp->number, utp->addr, "cmd", act, + encode_source_string (utp->number, utp->addr, "cmd", act.get (), buf, MAX_TRACE_UPLOAD); fprintf (writer->fp, "tp Z%s\n", buf); } diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c index c74e750..bed35bd 100644 --- a/gdb/tracepoint.c +++ b/gdb/tracepoint.c @@ -3083,7 +3083,8 @@ find_matching_tracepoint_location (struct uploaded_tp *utp) if (b->type == utp->type && t->step_count == utp->step && t->pass_count == utp->pass - && cond_string_is_same (t->cond_string, utp->cond_string) + && cond_string_is_same (t->cond_string, + utp->cond_string.get ()) /* FIXME also test actions. */ ) { @@ -3462,7 +3463,7 @@ parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp) int enabled, end; enum bptype type; const char *srctype; - char *cond, *buf; + char *buf; struct uploaded_tp *utp = NULL; p = line; @@ -3475,13 +3476,14 @@ parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp) p++; /* skip a colon */ if (piece == 'T') { + gdb::unique_xmalloc_ptr cond; + enabled = (*p++ == 'E'); p++; /* skip a colon */ p = unpack_varlen_hex (p, &step); p++; /* skip a colon */ p = unpack_varlen_hex (p, &pass); type = bp_tracepoint; - cond = NULL; /* Thumb through optional fields. */ while (*p == ':') { @@ -3502,8 +3504,8 @@ parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp) p++; p = unpack_varlen_hex (p, &xlen); p++; /* skip a comma */ - cond = (char *) xmalloc (2 * xlen + 1); - strncpy (cond, p, 2 * xlen); + cond.reset ((char *) xmalloc (2 * xlen + 1)); + strncpy (&cond[0], p, 2 * xlen); cond[2 * xlen] = '\0'; p += 2 * xlen; } @@ -3516,17 +3518,17 @@ parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp) utp->enabled = enabled; utp->step = step; utp->pass = pass; - utp->cond = cond; + utp->cond = std::move (cond); } else if (piece == 'A') { utp = get_uploaded_tp (num, addr, utpp); - utp->actions.push_back (xstrdup (p)); + utp->actions.emplace_back (xstrdup (p)); } else if (piece == 'S') { utp = get_uploaded_tp (num, addr, utpp); - utp->step_actions.push_back (xstrdup (p)); + utp->step_actions.emplace_back (xstrdup (p)); } else if (piece == 'Z') { @@ -3546,11 +3548,11 @@ parse_tracepoint_definition (const char *line, struct uploaded_tp **utpp) buf[end] = '\0'; if (startswith (srctype, "at:")) - utp->at_string = xstrdup (buf); + utp->at_string.reset (xstrdup (buf)); else if (startswith (srctype, "cond:")) - utp->cond_string = xstrdup (buf); + utp->cond_string.reset (xstrdup (buf)); else if (startswith (srctype, "cmd:")) - utp->cmd_strings.push_back (xstrdup (buf)); + utp->cmd_strings.emplace_back (xstrdup (buf)); } else if (piece == 'V') { diff --git a/gdb/tracepoint.h b/gdb/tracepoint.h index e62f7fd..62abb7c 100644 --- a/gdb/tracepoint.h +++ b/gdb/tracepoint.h @@ -178,21 +178,21 @@ struct uploaded_tp int orig_size = 0; /* String that is the encoded form of the tracepoint's condition. */ - char *cond = nullptr; + gdb::unique_xmalloc_ptr cond; /* Vectors of strings that are the encoded forms of a tracepoint's actions. */ - std::vector actions; - std::vector step_actions; + std::vector> actions; + std::vector> step_actions; /* The original string defining the location of the tracepoint. */ - char *at_string = nullptr; + gdb::unique_xmalloc_ptr at_string; /* The original string defining the tracepoint's condition. */ - char *cond_string = nullptr; + gdb::unique_xmalloc_ptr cond_string; /* List of original strings defining the tracepoint's actions. */ - std::vector cmd_strings; + std::vector> cmd_strings; /* The tracepoint's current hit count. */ int hit_count = 0; -- 2.7.4