Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leaks in qt/guiutil.cpp #11156

Merged
merged 1 commit into from Sep 7, 2017

Conversation

Projects
None yet
5 participants
@danra
Copy link
Contributor

danra commented Aug 26, 2017

on macOS:
listSnapshot was leaking in findStartupItemInList()
bitcoinAppUrl was leaking in [Get|Set]StartOnSystemStartup()

Fix memory leaks in qt/guiutil.cpp
on macOS:
listSnapshot was leaking in findStartupItemInList()
bitcoinAppUrl was leaking in [Get|Set]StartOnSystemStartup()

@laanwj laanwj added GUI macOS labels Aug 26, 2017

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 28, 2017

CFArrayGetCount dereferences listSnapshot and if it's nullptr then it crashes, so how have you tested this?

@danra

This comment has been minimized.

Copy link
Contributor Author

danra commented Aug 28, 2017

@promag Not sure I understand the question, do you mean

+    if (listSnapshot == nullptr) {
+        return nullptr;
+    }

added at 784-786?

This is just for safety, it's not really expected for listSnapshot to be nullptr. I think it makes sense to add this safety in the context of not doing CFRelease on nullptr at the end. You're right that even without the added lines above, in case of listSnapshot == nullptr there will probably be a crash even before we get to the CFRelease. Still, seems cleaner to me.

Also, in the unlikely case that listSnapshot is set to nullptr, we will return nullptr instead of crashing, which is probably nicer behavior anyway. (though, it's up to the caller whether it itself would crash or not)

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

utACK 9b348ff. Seems right, though maybe it would be simpler with an RAII helper class:

struct CFReleaser
{
    CFReleaser(CFTypeRef cf) m_cf(cf) {}
    ~CFReleaser() { if (m_cf) CFRelease(m_cf); }
    CFTypeRef m_cf;
};
@danra

This comment has been minimized.

Copy link
Contributor Author

danra commented Aug 30, 2017

@ryanofsky I agree, just didn't want to do two things at once - introduce such a class as well as fix problematic code.
Once this is merged I'm for doing a separate pull requests with a commit that introduces such a class, followed by commits which make use of it.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 3, 2017

utACK 9b348ff.
I think we should also get rid of the OSX Growl support.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 5, 2017

I really like the idea of using a RAII class here. Much easier to check whether all possible exit paths are covered, and exception-safety comes for free.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 7, 2017

I agree an RAII approach would be much better.
But until someone comes up with an RAII for that pure OSX stuff, I think this is an valid improvement.

Tested ACK 9b348ff.
Could not find any leaks with macOS instruments (leak detector) during notifications.

@jonasschnelli jonasschnelli merged commit 9b348ff into bitcoin:master Sep 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonasschnelli added a commit that referenced this pull request Sep 7, 2017

Merge #11156: Fix memory leaks in qt/guiutil.cpp
9b348ff Fix memory leaks in qt/guiutil.cpp (Dan Raviv)

Pull request description:

  on macOS:
  `listSnapshot` was leaking in `findStartupItemInList()`
  `bitcoinAppUrl` was leaking in `[Get|Set]StartOnSystemStartup()`

Tree-SHA512: dd49e1166336cf4f20035d21930f2f99f21f1d9f91a1101b1434a23dd0b92d402ac7efb177473c758d8af1dbab8d8750485583231c5b5854203d2493f0b43e73
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.