Closed Bug 1251213 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 81069)

Attachments

(1 file)

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 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.
Attachment #8723527 - Flags: review?(s.kaspari)
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/
Attachment #8723527 - Flags: review?(s.kaspari)
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 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.
Attachment #8723527 - Flags: review?(s.kaspari) → review+
yes that's why i've used it, i didn't want to have any discrepancies from the rest of the format.
https://hg.mozilla.org/mozilla-central/rev/5b643385e0da
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.