1251213 - [Static Analysis][Resource leak] In DoCommand::UpdateCallBack

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andi [:andi]]

The Static Analysis tool Coverity added that variable |fis| can leak memory in the following context:

>>            FileInputStream fis = ctx.openFileInput(sFileName);
>>            int nBytes = fis.available();
>>            if (nBytes > 0)
>>                {
>>                byte [] buffer = new byte [nBytes + 1];
>>                int nRead = fis.read(buffer, 0, nBytes);
>>                fis.close();
>>                ctx.deleteFile(sFileName);
>>                if (nRead > 0)
>>                    {
>>                    String sBuffer = new String(buffer);
>>                    nEnd = sBuffer.indexOf(',');
>>                    if (nEnd > 0)
>>                        {
>>                        sIP = (sBuffer.substring(0, nEnd)).trim();
>>                        nStart = nEnd + 1;
>>                        nEnd = sBuffer.indexOf('\r', nStart);
>>                        if (nEnd > 0)
>>                            {
>>                            sPort = (sBuffer.substring(nStart, nEnd)).trim();
>>                            Thread.sleep(5000);
>>                            sRet = RegisterTheDevice(sIP, sPort, sBuffer.substring(nEnd + 1));
>>                            }
>>                        }
>>                    }
>>                }

I think there are two possibilities here in order to have a memory leak:

  1. nBytes is <= 0, as Coverity states
  2. fis.available() raises an IOException

For this i consider that we should add |fis| in a try-with-resources statement.If we target devices lower than api level 19 i can reupdate the patch with:
>>    finally
>>          {          
>>        try 
>>          {
>>          fis.close();
>>          }
>>        }

Comment 1

[Andi [:andi]]

Attachment: MozReview Request: Bug 1251213 - release resources from fis. r?sebastian

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

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8723527 [details]
MozReview Request: Bug 1251213 - release resources from fis. r?sebastian

https://reviewboard.mozilla.org/r/36587/#review33699

Did you test this on an older device - preferably a Gingerbread one? It seems like all articles I found point to a minSdkVersion of 19 for try-with-resources.

Feel free to reflag me if this work as is.

Comment 3

[Andi [:andi]]

Comment on attachment 8723527 [details]
MozReview Request: Bug 1251213 - release resources from fis. r?sebastian

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

Comment 4

[Andi [:andi]]

I've reverted my previous change since that was only valid for API Level 19+.

I've added fis.close in a try() catch() block since close throws an exception.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8723527 [details]
MozReview Request: Bug 1251213 - release resources from fis. r?sebastian

https://reviewboard.mozilla.org/r/36587/#review34465

The format looks weird but it seems to follow what is used in this file.

Comment 6

[Andi [:andi]]

yes that's why i've used it, i didn't want to have any discrepancies from the rest of the format.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b643385e0da

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b643385e0da