[Static Analysis][Resource leak] In function query (Uri mainUri, Uri fallbackUri, String condition)

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: andi, Assigned: mcomella)

Tracking

(Blocks: 2 bugs, {coverity})

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

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: CID 121161)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
The Static Analysis tool Coverity added that a memory leak can occur in the following code:

>>        Cursor cursor = mCr.query(mainUri, null, condition, null, null);
>>
>>        if (Build.MANUFACTURER.equals(SAMSUNG_MANUFACTURER) && (cursor == null || cursor.getCount() == 0)) {
>>            cursor = mCr.query(fallbackUri, null, null, null, null);
>>        }

I think this is possible only if cursor is a valid object. We should do a check inside the if branch for cursor validity and if so close it.
(Reporter)

Comment 1

3 years ago
Created attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Review commit: https://reviewboard.mozilla.org/r/34487/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34487/
Attachment #8718281 - Flags: review?(mark.finkle)
Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Passing to Mike. He's a better reviewer for this.
Attachment #8718281 - Flags: review?(mark.finkle) → review?(michael.l.comella)
Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

https://reviewboard.mozilla.org/r/34487/#review31791

Looks good – just a small style nit.

Thanks for the patch!

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:158
(Diff revision 1)
> +            if ( cursor != null )
> +                cursor.close();

nit: Our style is `if (expression) {`. Also, we always brace if statements.
Attachment #8718281 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

https://reviewboard.mozilla.org/r/34487/#review31795

Actually, this should really be in a `try {} finally { Cursor.close(); }` since it's safer. Can you figure out a way to shoe-horn the code into that? Perhaps with some temp variables.
Attachment #8718281 - Flags: review+
(Reporter)

Comment 5

3 years ago
Hello Michael, i'm not a java expert but as far as i can see query doesn't throw any exception so i don't see the point of using try {} finally {}. The exceptions that can be triggered inside query are handled locally by that function.
If there is something that i didn't get let me know.
Flags: needinfo?(michael.l.comella)
(In reply to Andi-Bogdan Postelnicu from comment #5)
> Hello Michael, i'm not a java expert but as far as i can see query doesn't
> throw any exception so i don't see the point of using try {} finally {}. The
> exceptions that can be triggered inside query are handled locally by that
> function.
> If there is something that i didn't get let me know.

It's a defensive coding technique – there are no Exceptions thrown at the moment but will the code throw Exceptions after a future developer adds code? Additionally, the code can still throw RuntimeExceptions that don't need to be explicitly rethrown  by the method (e.g. NullPointerException), which the calling code can (questionably) catch and recover from, in which case the resource would be left open.
Flags: needinfo?(michael.l.comella)
(Reporter)

Comment 7

3 years ago
Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34487/diff/1-2/
Attachment #8718281 - Flags: review?(mark.finkle)
(Reporter)

Comment 8

3 years ago
Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34487/diff/2-3/
Attachment #8718281 - Attachment description: MozReview Request: Bug 1247557 - close cursor if it's valid before doing a new query. r?mfinkle → MozReview Request: Bug 1247557 - avoid memory leak by closing cursorFirst at the final of the function if cursorSecond has been created. r?mcomella
Attachment #8718281 - Flags: review?(mark.finkle) → review?(michael.l.comella)
(Reporter)

Updated

3 years ago
Attachment #8718281 - Flags: review?(s.kaspari)
Attachment #8718281 - Flags: review?(s.kaspari)
Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

https://reviewboard.mozilla.org/r/34487/#review32091

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:161
(Diff revision 3)
> -            cursor = mCr.query(fallbackUri, null, null, null, null);
> +                cursorSecond = mCr.query(fallbackUri, null, null, null, null);

Isn't this the only situation where you need to close the previous cursor if it is not null? The code might be simpler if you close it here.

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:164
(Diff revision 3)
> +        catch (Exception e) {
> +            Log.e(LOGTAG, "Exception while querying db: ", e);
> +        }

You can have try { } finaly { } blocks without catching an exception.
(Reporter)

Comment 10

3 years ago
Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34487/diff/3-4/
(Reporter)

Comment 11

3 years ago
Comment on attachment 8718281 [details]
MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34487/diff/4-5/
Attachment #8718281 - Attachment description: MozReview Request: Bug 1247557 - avoid memory leak by closing cursorFirst at the final of the function if cursorSecond has been created. r?mcomella → MozReview Request: Bug 1247557 - avoid memory leak by closing cursor before being initialized for the 2nd time. r?mcomella
Hey Andi-Bogdan.

bug 1247636 and this one are some pretty complex error cases and, as you mentioned you were not a Java expert, I'd like to avoid the back-and-forth review cycle if it bothers you. Would you like me to take over finishing these bugs?

In any case, I really appreciate your help and your watch over our Coverity defects! :)
Flags: needinfo?(bogdan.postelnicu)
(Reporter)

Comment 13

3 years ago
Hey Michael,

No problem for me, do as you consider it's best.
Flags: needinfo?(bogdan.postelnicu)
Created attachment 8722200 [details]
MozReview Request: Bug 1247557 - Close Cursors in AndroidImport. r=sebastian

Review commit: https://reviewboard.mozilla.org/r/35897/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35897/
Attachment #8722200 - Flags: review?(s.kaspari)
Assignee: bogdan.postelnicu → michael.l.comella
Attachment #8718281 - Attachment is obsolete: true
Attachment #8718281 - Flags: review?(michael.l.comella)
Comment on attachment 8722200 [details]
MozReview Request: Bug 1247557 - Close Cursors in AndroidImport. r=sebastian

https://reviewboard.mozilla.org/r/35897/#review32663

I don't think the catch is needed, see below. Without that r+. :)

::: mobile/android/base/java/org/mozilla/gecko/preferences/AndroidImport.java:165
(Diff revision 1)
> +        } catch (final Exception e) {
> +            if (cursor != null) {
> +                cursor.close();
> +            }
> +            throw e;
> +        }

Isn't this already (and shouldn't this) be handled by the callers? They all wrap their calls to query() in try..finally and always close the cursor.
Attachment #8722200 - Flags: review?(s.kaspari) → review+
https://reviewboard.mozilla.org/r/35897/#review32663

> Isn't this already (and shouldn't this) be handled by the callers? They all wrap their calls to query() in try..finally and always close the cursor.

But if `query` throws, the `Cursor` will not be returned so the callers can't close it.
https://reviewboard.mozilla.org/r/35897/#review32663

> But if `query` throws, the `Cursor` will not be returned so the callers can't close it.

Spoke with rnewman – the catch complicates the code path so much more than the probability that someone will come along and change the code, miss the leaked Cursor in review, and we won't catch it again in static analysis that it's not worth adding in the extra lines.

Comment 19

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