From 7fc72b4e1b255dd4a346360376e0a06f1c5a452c Mon Sep 17 00:00:00 2001 From: Andrzej Hunt Date: Wed, 19 Apr 2017 08:26:38 -0700 Subject: [PATCH] Bug 1356693 - infer: fix RESOURCE_LEAK's in base r=walkingice MozReview-Commit-ID: Gm9GqOk37UZ --HG-- extra : rebase_source : 7f6a1716e31dae1cc1e46dbe19a12f45edc05b89 --- .../java/org/mozilla/gecko/ANRReporter.java | 7 +++- .../org/mozilla/gecko/db/SuggestedSites.java | 16 ++++----- .../gecko/distribution/Distribution.java | 11 +++++- .../gecko/switchboard/SwitchBoard.java | 20 ++++++++--- .../mozilla/gecko/updater/UpdateService.java | 36 +++++++++++++------ 5 files changed, 62 insertions(+), 28 deletions(-) diff --git a/mobile/android/base/java/org/mozilla/gecko/ANRReporter.java b/mobile/android/base/java/org/mozilla/gecko/ANRReporter.java index 7893de0843ae..c18431166b63 100644 --- a/mobile/android/base/java/org/mozilla/gecko/ANRReporter.java +++ b/mobile/android/base/java/org/mozilla/gecko/ANRReporter.java @@ -22,6 +22,7 @@ import java.util.regex.Pattern; import org.json.JSONObject; import org.mozilla.gecko.annotation.WrapForJNI; import org.mozilla.gecko.AppConstants.Versions; +import org.mozilla.gecko.util.IOUtils; import org.mozilla.gecko.util.StringUtils; import org.mozilla.gecko.util.ThreadUtils; @@ -154,8 +155,10 @@ public final class ANRReporter extends BroadcastReceiver .command("/system/bin/getprop", "dalvik.vm.stack-trace-file") .redirectErrorStream(true) .start(); + + BufferedReader buf = null; try { - BufferedReader buf = new BufferedReader( + buf = new BufferedReader( new InputStreamReader( propProc.getInputStream(), StringUtils.UTF_8), TRACES_LINE_SIZE); String propVal = buf.readLine(); @@ -176,6 +179,8 @@ public final class ANRReporter extends BroadcastReceiver } } finally { propProc.destroy(); + + IOUtils.safeStreamClose(buf); } } catch (IOException e) { Log.w(LOGTAG, e); diff --git a/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java b/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java index 89b12904bb3c..929e93d3d313 100644 --- a/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java +++ b/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java @@ -41,7 +41,9 @@ import org.mozilla.gecko.Locales; import org.mozilla.gecko.R; import org.mozilla.gecko.distribution.Distribution; import org.mozilla.gecko.restrictions.Restrictions; +import org.mozilla.gecko.util.IOUtils; import org.mozilla.gecko.util.RawResource; +import org.mozilla.gecko.util.StringUtils; import org.mozilla.gecko.util.ThreadUtils; import org.mozilla.gecko.preferences.GeckoPreferences; @@ -268,7 +270,7 @@ public class SuggestedSites { return; } - OutputStreamWriter osw = null; + OutputStreamWriter outputStreamWriter = null; try { final JSONArray jsonSites = new JSONArray(); @@ -276,20 +278,14 @@ public class SuggestedSites { jsonSites.put(site.toJSON()); } - osw = new OutputStreamWriter(new FileOutputStream(f), "UTF-8"); + outputStreamWriter = new OutputStreamWriter(new FileOutputStream(f), StringUtils.UTF_8); final String jsonString = jsonSites.toString(); - osw.write(jsonString, 0, jsonString.length()); + outputStreamWriter.write(jsonString, 0, jsonString.length()); } catch (Exception e) { Log.e(LOGTAG, "Failed to save suggested sites", e); } finally { - if (osw != null) { - try { - osw.close(); - } catch (IOException e) { - // Ignore. - } - } + IOUtils.safeStreamClose(outputStreamWriter); } } diff --git a/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java b/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java index b765c05d7042..12920423eb2d 100644 --- a/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java +++ b/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java @@ -649,7 +649,16 @@ public class Distribution { return null; } - return new JarInputStream(new BufferedInputStream(connection.getInputStream()), true); + final BufferedInputStream bufferedInputStream = new BufferedInputStream(connection.getInputStream()); + try { + return new JarInputStream(bufferedInputStream, true); + } catch (IOException e) { + // Thrown e.g. if JarInputStream can't parse the input as a valid Zip. + // In that case we need to ensure the bufferedInputStream gets closed since it won't + // be used anywhere (while still passing the Exception up the stack). + bufferedInputStream.close(); + throw e; + } } private static void recordFetchTelemetry(final Exception exception) { diff --git a/mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java b/mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java index 685e0b172238..763ef685e78a 100644 --- a/mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java +++ b/mobile/android/base/java/org/mozilla/gecko/switchboard/SwitchBoard.java @@ -34,6 +34,7 @@ import org.json.JSONObject; import org.json.JSONArray; import org.mozilla.gecko.AppConstants; import org.mozilla.gecko.util.HardwareUtils; +import org.mozilla.gecko.util.IOUtils; import org.mozilla.gecko.util.ProxySelector; import android.content.Context; @@ -378,27 +379,36 @@ public class SwitchBoard { * @return Returns String from server or null when failed. */ @Nullable private static String readFromUrlGET(URL url) { + HttpURLConnection connection = null; + InputStreamReader inputStreamReader = null; + BufferedReader bufferReader = null; try { - HttpURLConnection connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(url.toURI()); + connection = (HttpURLConnection) ProxySelector.openConnectionWithProxy(url.toURI()); connection.setRequestProperty("User-Agent", HardwareUtils.isTablet() ? AppConstants.USER_AGENT_FENNEC_TABLET : AppConstants.USER_AGENT_FENNEC_MOBILE); connection.setRequestMethod("GET"); connection.setUseCaches(false); - InputStream is = connection.getInputStream(); - InputStreamReader inputStreamReader = new InputStreamReader(is); - BufferedReader bufferReader = new BufferedReader(inputStreamReader, 8192); + // BufferedReader(Reader, int) can throw, hence we need to keep a separate reference + // to the InputStreamReader in order to always be able to close it: + inputStreamReader = new InputStreamReader(connection.getInputStream()); + bufferReader = new BufferedReader(inputStreamReader, 8192); String line; StringBuilder resultContent = new StringBuilder(); while ((line = bufferReader.readLine()) != null) { resultContent.append(line); } - bufferReader.close(); return resultContent.toString(); } catch (IOException | URISyntaxException e) { e.printStackTrace(); + } finally { + IOUtils.safeStreamClose(bufferReader); + IOUtils.safeStreamClose(inputStreamReader); + if (connection != null) { + connection.disconnect(); + } } return null; diff --git a/mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java b/mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java index 7ccc43e28255..8a185445873b 100644 --- a/mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java +++ b/mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java @@ -12,6 +12,7 @@ import org.mozilla.gecko.R; import org.mozilla.apache.commons.codec.binary.Hex; import org.mozilla.gecko.permissions.Permissions; +import org.mozilla.gecko.util.IOUtils; import org.mozilla.gecko.util.ProxySelector; import org.w3c.dom.Document; import org.w3c.dom.Node; @@ -48,6 +49,7 @@ import java.io.FileInputStream; import java.io.FileOutputStream; import java.io.InputStream; import java.io.OutputStream; +import java.net.HttpURLConnection; import java.net.URI; import java.net.URL; import java.net.URLConnection; @@ -373,6 +375,7 @@ public class UpdateService extends IntentService { } private UpdateInfo findUpdate(boolean force) { + URLConnection conn = null; try { URI uri = getUpdateURI(force); @@ -382,7 +385,8 @@ public class UpdateService extends IntentService { } DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder(); - Document dom = builder.parse(ProxySelector.openConnectionWithProxy(uri).getInputStream()); + conn = ProxySelector.openConnectionWithProxy(uri); + Document dom = builder.parse(conn.getInputStream()); NodeList nodes = dom.getElementsByTagName("update"); if (nodes == null || nodes.getLength() == 0) @@ -432,6 +436,14 @@ public class UpdateService extends IntentService { } catch (Exception e) { Log.e(LOGTAG, "failed to check for update: ", e); return null; + } finally { + // conn isn't guaranteed to be an HttpURLConnection, hence we don't want to cast earlier + // in this method. However in our current implementation it usually is, so we need to + // make sure we close it in that case: + final HttpURLConnection httpConn = (HttpURLConnection) conn; + if (httpConn != null) { + httpConn.disconnect(); + } } } @@ -551,6 +563,7 @@ public class UpdateService extends IntentService { OutputStream output = null; InputStream input = null; + URLConnection conn = null; mDownloading = true; mCancelDownload = false; @@ -563,7 +576,7 @@ public class UpdateService extends IntentService { mWifiLock.acquire(); } - URLConnection conn = ProxySelector.openConnectionWithProxy(info.uri); + conn = ProxySelector.openConnectionWithProxy(info.uri); int length = conn.getContentLength(); output = new BufferedOutputStream(new FileOutputStream(downloadFile)); @@ -606,21 +619,22 @@ public class UpdateService extends IntentService { Log.e(LOGTAG, "failed to download update: ", e); return null; } finally { - try { - if (input != null) - input.close(); - } catch (java.io.IOException e) { } - - try { - if (output != null) - output.close(); - } catch (java.io.IOException e) { } + IOUtils.safeStreamClose(input); + IOUtils.safeStreamClose(output); mDownloading = false; if (mWifiLock.isHeld()) { mWifiLock.release(); } + + // conn isn't guaranteed to be an HttpURLConnection, hence we don't want to cast earlier + // in this method. However in our current implementation it usually is, so we need to + // make sure we close it in that case: + final HttpURLConnection httpConn = (HttpURLConnection) conn; + if (httpConn != null) { + httpConn.disconnect(); + } } }