1261983 - [infer] Resource leaks in dlc

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

/var/lib/jenkins/workspace/fennec-infer/mobile/android/base/java/org/mozilla/gecko/dlc/BaseAction.java:142: error: RESOURCE_LEAK
   resource acquired by call to FileInputStream(...) at line 115 is not released after line 142

/var/lib/jenkins/workspace/fennec-infer/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:190: error: RESOURCE_LEAK
   resource acquired by call to openFile(...) at line 179 is not released after line 190

/var/lib/jenkins/workspace/fennec-infer/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:216: error: RESOURCE_LEAK
   resource acquired by call to FileInputStream(...) at line 216 is not released after line 216

/var/lib/jenkins/workspace/fennec-infer/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:234: error: RESOURCE_LEAK
   resource acquired by call to FileInputStream(...) at line 216 is not released after line 234

/var/lib/jenkins/workspace/fennec-infer/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:306: error: RESOURCE_LEAK
   resource acquired by call to FileInputStream(...) at line 294 is not released after line 306

/var/lib/jenkins/workspace/fennec-infer/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:307: error: RESOURCE_LEAK
   resource acquired by call to FileOutputStream(...) at line 295 is not released after line 307

Sebastian, mind taking a look?

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Thank you! I'll have a look.

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

This looks like false positives.

The tool doesn't seem to recognize our helper method IOUtils.safeStreamClose() as a way to close the stream. Doesn't it follow the code inside the methods? Can we teach/configure the tool to do so?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Example:

> /var/lib/jenkins/workspace/fennec-infer/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java:190: error: RESOURCE_LEAK
>   resource acquired by call to openFile(...) at line 179 is not released after line 190

Code:
  https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/dlc/DownloadAction.java#189-190

Comment 4

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> This looks like false positives.
>
> The tool doesn't seem to recognize our helper method
> IOUtils.safeStreamClose() as a way to close the stream. Doesn't it follow
> the code inside the methods? Can we teach/configure the tool to do so?

So they're open source: https://github.com/facebook/infer/ (I hope you've kept up with your OCaml ;)

And they have a bug open for this functionality: https://github.com/facebook/infer/issues/246

That being said:
 * Beyond whitelisting/blacklisting files [1], I can't find a way to ignore individual checks. There is an undocumented `.inferconfig` file – perhaps there's something in there? Worst case, we can filter the output to ignore the checks we don't want to see anymore (we'll have more control over this once we add infer locally but that might be some ways off).
 * The docs mentions dynamic dispatch is something they can't do (yet?) [2] – I wonder if that applies to static methods?

I'm not sure what the best course of action is – maybe just close this bug, filter out these checks in my script, and hope it gets fixed soon? :P

[1]: https://github.com/facebook/infer/issues/312
[2]: http://fbinfer.com/docs/limitations.html

Comment 5

[Michael Comella (:mcomella) [NI reported issues only; no longer employed by Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #4)
> And they have a bug open for this functionality:
> https://github.com/facebook/infer/issues/246

Oops, this is actually specific to code in that issue.

That being said, I found out you can use @SuppressWarnings("infer") [1] at the method level and they should work in gradle on more recent versions – unclear what we're running in "automation".

[1]: https://github.com/facebook/infer/issues/150#issuecomment-141204309
[2]: https://github.com/facebook/infer/issues/150#issuecomment-199320293

Comment 6

[Andrzej Hunt :ahunt]

FWIW it looks like most of the issues have disappeared with the current version of infer, however there is still one issue.

Comment 7

[Andrzej Hunt :ahunt]

Attachment: Bug 1261983 - infer: close file input stream in case of GZIP failure

This commit also cleans up extraneous stream closes: these streams
are close in finally, so we don't need to also clean them
up in the main try block body.

Review commit: https://reviewboard.mozilla.org/r/130350/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130350/

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8858401 [details]
Bug 1261983 - infer: close file input stream in case of GZIP failure

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130350/diff/1-2/

Comment 9

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5b67af192ba8b8d0abbdb0f03bde9809da626d2
Bug 1261983 - infer: close file input stream in case of GZIP failure r=nechen

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5b67af192ba