[RFC] Fix BFD leak in solib-darwin.c

[RFC] Fix BFD leak in solib-darwin.c

[Tom Tromey]

commit 192b62ce0b4bb5c61188f570e127a26d2c32f716 ("Use class to manage
BFD reference counts") changed darwin_get_dyld_bfd to use:

+	dyld_bfd.release ();

rather than

-      do_cleanups (cleanup);

However, using release here leaks the BFD.  Instead I believe reset
should be used instead.

I can't readily test this, so please take a look and let me know what
you think.

gdb/ChangeLog
2019-02-22  Tom Tromey  <tromey@adacore.com>

	* solib-darwin.c (darwin_get_dyld_bfd): Use reset, not release.
---
 gdb/ChangeLog      | 4 ++++
 gdb/solib-darwin.c | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
index d3060604bad..da410ad4660 100644
--- a/gdb/solib-darwin.c
+++ b/gdb/solib-darwin.c
@@ -455,7 +455,7 @@ darwin_get_dyld_bfd ()
       if (sub != NULL)
 	dyld_bfd = sub;
       else
-	dyld_bfd.release ();
+	dyld_bfd.reset (nullptr);
     }
   return dyld_bfd;
 }
-- 
2.20.1

Re: [RFC] Fix BFD leak in solib-darwin.c

[Pedro Alves]

On 02/22/2019 04:36 PM, Tom Tromey wrote:
>        if (sub != NULL)
>  	dyld_bfd = sub;
>        else
> -	dyld_bfd.release ();
> +	dyld_bfd.reset (nullptr);

These 4 lines are now equivalent to just doing:

   dyld_bfd = sub;

Thanks,
Pedro Alves

Re: [RFC] Fix BFD leak in solib-darwin.c

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> On 02/22/2019 04:36 PM, Tom Tromey wrote:
>> if (sub != NULL)
>> dyld_bfd = sub;
>> else
>> -	dyld_bfd.release ();
>> +	dyld_bfd.reset (nullptr);

Pedro> These 4 lines are now equivalent to just doing:

Pedro>    dyld_bfd = sub;

I made this change and I'm going to commit it shortly.

Tom

Re: [RFC] Fix BFD leak in solib-darwin.c

[John Baldwin]

On 2/22/19 9:16 AM, John Baldwin wrote:
> On 2/22/19 8:36 AM, Tom Tromey wrote:
>> commit 192b62ce0b4bb5c61188f570e127a26d2c32f716 ("Use class to manage
>> BFD reference counts") changed darwin_get_dyld_bfd to use:
>>
>> +	dyld_bfd.release ();
>>
>> rather than
>>
>> -      do_cleanups (cleanup);
>>
>> However, using release here leaks the BFD.  Instead I believe reset
>> should be used instead.
>>
>> I can't readily test this, so please take a look and let me know what
>> you think.
>>
>> gdb/ChangeLog
>> 2019-02-22  Tom Tromey  <tromey@adacore.com>
>>
>> 	* solib-darwin.c (darwin_get_dyld_bfd): Use reset, not release.
>> ---
>>  gdb/ChangeLog      | 4 ++++
>>  gdb/solib-darwin.c | 2 +-
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/gdb/solib-darwin.c b/gdb/solib-darwin.c
>> index d3060604bad..da410ad4660 100644
>> --- a/gdb/solib-darwin.c
>> +++ b/gdb/solib-darwin.c
>> @@ -455,7 +455,7 @@ darwin_get_dyld_bfd ()
>>        if (sub != NULL)
>>  	dyld_bfd = sub;
>>        else
>> -	dyld_bfd.release ();
>> +	dyld_bfd.reset (nullptr);
>>      }
>>    return dyld_bfd;
>>  }
> 
> I haven't tested, but this LGTM.  Simon has noticed several instances of
> this bug where release() was used instead of reset().  (The most recent one
> was in the build-id separate debug file code that I think hasn't been
> committed yet.)  It might be worth doing a quick sweep of current 'release'
> calls (if there aren't too many) to check for other leaks.

I did a quick look and this is the only other one I could find that I think
is also a leak:

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index 98f46e0416..055cbc8073 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -2722,7 +2722,7 @@ dwarf2_get_dwz_file (struct dwarf2_per_objfile *dwarf2_per_objfile)
   if (dwz_bfd != NULL)
     {
       if (!build_id_verify (dwz_bfd.get (), buildid_len, buildid))
-       dwz_bfd.release ();
+       dwz_bfd.reset (nullptr);
     }
 
   if (dwz_bfd == NULL)


-- 
John Baldwin

                                                                            

Re: [RFC] Fix BFD leak in solib-darwin.c

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> I did a quick look and this is the only other one I could find that I think
John> is also a leak:

Thanks for doing this.  I agree with your analysis, please write a
ChangeLog entry & check it in.

Tom

Re: [RFC] Fix BFD leak in solib-darwin.c

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

Re-replying...

John> Simon has noticed several instances of this bug where release()
John> was used instead of reset().

I wonder whether we ought to mark the release method as
"warn_unused_result" to try to avoid future bugs like this.  Another
idea is to try to reduce the number of calls to release generally, by
changing more things to take rvalue references or the like.

Tom

Re: [RFC] Fix BFD leak in solib-darwin.c

[John Baldwin]

On 2/26/19 12:18 PM, Tom Tromey wrote:
>>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:
> 
> Re-replying...
> 
> John> Simon has noticed several instances of this bug where release()
> John> was used instead of reset().
> 
> I wonder whether we ought to mark the release method as
> "warn_unused_result" to try to avoid future bugs like this.  Another
> idea is to try to reduce the number of calls to release generally, by
> changing more things to take rvalue references or the like.

I think the warning would be useful if it doesn't trigger a lot of false
positives.  I feel like it wouldn't today.  When I was grep'ing for
'release ()' I only focused on the calls that weren't assigning a value
to a result and I feel like there weren't many of those (and some were
assigning, just not obvious in the one line of grep context).

-- 
John Baldwin

                                                                            

Re: [RFC] Fix BFD leak in solib-darwin.c

[Tom Tromey]

>>>>> "John" == John Baldwin <jhb@FreeBSD.org> writes:

John> I think the warning would be useful if it doesn't trigger a lot of false
John> positives.  I feel like it wouldn't today.  When I was grep'ing for
John> 'release ()' I only focused on the calls that weren't assigning a value
John> to a result and I feel like there weren't many of those (and some were
John> assigning, just not obvious in the one line of grep context).

Yeah.  I went ahead and did this... I had to fix up a few spots, nothing
too terrible.  It caught one bug, in varobj.c:

      if (pretty_printer == Py_None)
	pretty_printer.release ();

      install_visualizer (var->dynamic, NULL, pretty_printer.release ());

I think that first release should be .reset(nullptr) instead.

I'll send it through the buildbot and see what happens there.

Tom