diff mbox

Fix resource leak found by Coverity

Message ID 1539179578-32250-1-git-send-email-gbenson@redhat.com
State New, archived
Headers

Commit Message

Gary Benson Oct. 10, 2018, 1:52 p.m. UTC 
  Hi all,

This commit fixes a resource leak found by Coverity where
find_oload_champ leaked every badness vector it calculated
bar the one it returned.

Built and regtested on RHEL 7.5 x86_64.

Ok to commit?

Thanks,
Gary

--
gdb/ChangeLog:

	* valops.c (find_oload_champ): Fix resource leak found by
	Coverity.
---
 gdb/ChangeLog | 5 +++++
 gdb/valops.c  | 9 ++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

Comments

Pedro Alves Oct. 10, 2018, 2:11 p.m. UTC | #1 
Please use some more descriptive subject in these emails/commits,
otherwise we're going to end up with dozens of indistinct 
"Fix resource leak found by Coverity" patches/emails.  :-)

E.g., "Fix leak in valops.c:find_oload_champ" or some such.

As for the patch itself, I think that we'd still be leaking bv->rank,
right?  Not just here, but in the callers of rank_function
as well?

I have a patch from last year here that converts the badness
vector to a C++ std::vector:

 https://github.com/palves/gdb/commits/palves/badness_vector

See top two commits.

I guess a better approach would be to get that in instead.

Simon had a badness_vector C++ification patch too,
though his was different.  ISTR that I prefer my approach,
but I won't be surprised if Simon preferred his.  :-)

Thanks,
Pedro Alves
Gary Benson Oct. 10, 2018, 2:20 p.m. UTC | #2 
Pedro Alves wrote:
> Please use some more descriptive subject in these emails/commits,
> otherwise we're going to end up with dozens of indistinct 
> "Fix resource leak found by Coverity" patches/emails.  :-)

Yeah, it just occurred to me to do that, after I looked in my
inbox :D

> As for the patch itself, I think that we'd still be leaking
> bv->rank, right?  Not just here, but in the callers of rank_function
> as well?

Oh, I just looked in valops.c to see what other things did to
free the vectors.  So there's other leaks in there as well :/

> I have a patch from last year here that converts the badness
> vector to a C++ std::vector:
> 
>  https://github.com/palves/gdb/commits/palves/badness_vector
> 
> See top two commits.
> 
> I guess a better approach would be to get that in instead.
> 
> Simon had a badness_vector C++ification patch too,
> though his was different.  ISTR that I prefer my approach,
> but I won't be surprised if Simon preferred his.  :-)

I'll look at them.  Were they ever submitted, or unfinished, or... ?

Cheers,
Gary
diff mbox

Patch

diff --git a/gdb/valops.c b/gdb/valops.c
index c45caef..db2d3ed 100644
--- a/gdb/valops.c
+++ b/gdb/valops.c
@@ -3024,8 +3024,6 @@  find_oload_champ (struct value **args, int nargs,
 		  struct badness_vector **oload_champ_bv)
 {
   int ix;
-  /* A measure of how good an overloaded instance is.  */
-  struct badness_vector *bv;
   /* Index of best overloaded function.  */
   int oload_champ = -1;
   /* Current ambiguity state for overload resolution.  */
@@ -3075,13 +3073,15 @@  find_oload_champ (struct value **args, int nargs,
 
       /* Compare parameter types to supplied argument types.  Skip
          THIS for static methods.  */
-      bv = rank_function (parm_types, nparms, 
+      struct badness_vector *bv
+	= rank_function (parm_types, nparms,
 			  args + static_offset,
 			  nargs - static_offset);
 
       if (!*oload_champ_bv)
 	{
 	  *oload_champ_bv = bv;
+	  bv = NULL;
 	  oload_champ = 0;
 	}
       else /* See whether current candidate is better or worse than
@@ -3095,7 +3095,9 @@  find_oload_champ (struct value **args, int nargs,
 	    oload_ambiguous = 2;
 	    break;
 	  case 2:		/* New champion, record details.  */
+	    xfree (*oload_champ_bv);
 	    *oload_champ_bv = bv;
+	    bv = NULL;
 	    oload_ambiguous = 0;
 	    oload_champ = ix;
 	    break;
@@ -3103,6 +3105,7 @@  find_oload_champ (struct value **args, int nargs,
 	  default:
 	    break;
 	  }
+      xfree (bv);
       xfree (parm_types);
       if (overload_debug)
 	{