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

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks 1 bug, {coverity})

unspecified
Firefox 47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: CID 81069)

Attachments

(1 attachment)

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)
Assignee

Comment 3

3 years ago
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)
Assignee

Comment 4

3 years ago
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+
Assignee

Comment 6

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

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b643385e0da
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.