From patchwork Fri Dec 14 19:12:42 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 30673 Received: (qmail 12317 invoked by alias); 14 Dec 2018 19:12:53 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 12182 invoked by uid 89); 14 Dec 2018 19:12:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=fat, tracepoint, tracepoints, u32 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 14 Dec 2018 19:12:47 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8DB55C0AF785 for ; Fri, 14 Dec 2018 19:12:46 +0000 (UTC) Received: from localhost.localdomain (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0627D5D756 for ; Fri, 14 Dec 2018 19:12:45 +0000 (UTC) From: Pedro Alves To: gdb-patches@sourceware.org Subject: [PATCH 3/3] Fix tracepoint.c:parse_tracepoint_definition leak (and one more) Date: Fri, 14 Dec 2018 19:12:42 +0000 Message-Id: <20181214191242.21560-4-palves@redhat.com> In-Reply-To: <20181214191242.21560-1-palves@redhat.com> References: <20181214191242.21560-1-palves@redhat.com> 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. Let me know if you have a strong preference otherwise. gdb/ChangeLog: 2018-12-14 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/breakpoint.c | 6 +++--- gdb/ctf.c | 30 +++++++++++++++++------------- gdb/tracefile-tfile.c | 21 +++++++++++---------- gdb/tracepoint.c | 24 +++++++++++++----------- gdb/tracepoint.h | 12 ++++++------ 5 files changed, 50 insertions(+), 43 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 8af3d54a77..4a0b8dc0dc 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -14681,7 +14681,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++; } @@ -14702,7 +14702,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 @@ -14726,7 +14726,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 ca5266fbd7..e98fb9c1ba 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 (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 (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 (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 6996e3d1e5..8e8cb15fd4 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 (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 (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 (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 8cd53374f9..4c940b0014 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 b43fbd7606..fbde53ebf4 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;