[Static Analysis][Resource leak] In function Classifier::GetLookupCache

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla47
coverity
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: CID 749516)

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
The Static Analysis tool Coverity added that pointer |cache| can leak the memory to witch points in two situations:

1.
>>  LookupCache *cache = new LookupCache(aTable, mStoreDirectory);
>>  nsresult rv = cache->Init();
>>  if (NS_FAILED(rv)) {
>>    return nullptr;
>>  }

2.

>>  rv = cache->Open();
>>  if (NS_FAILED(rv)) {
>>    if (rv == NS_ERROR_FILE_CORRUPTED) {
>>      Reset();
>>    }
>>    return nullptr;
>>  }

A simple delete cache, in both ifs, should solve the 2 issues.
(Assignee)

Updated

3 years ago
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
(Assignee)

Comment 1

3 years ago
Created attachment 8722428 [details]
MozReview Request: Bug 1250439 - preven memory leak in Classifier::GetLookupCache. r?gcp

Review commit: https://reviewboard.mozilla.org/r/36025/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36025/
Attachment #8722428 - Flags: review?(dietrich)
(Assignee)

Updated

3 years ago
Component: General → Safe Browsing
GCP are you the right person to review this?
Flags: needinfo?(gpascutto)
https://reviewboard.mozilla.org/r/36025/#review32711

::: toolkit/components/url-classifier/Classifier.cpp:695
(Diff revision 1)
>    LookupCache *cache = new LookupCache(aTable, mStoreDirectory);

Let's not repeat mistakes and use a RefPtr here?
Flags: needinfo?(gpascutto)
(Assignee)

Comment 4

3 years ago
I'll update the patch and report it.
(Assignee)

Comment 5

3 years ago
Comment on attachment 8722428 [details]
MozReview Request: Bug 1250439 - preven memory leak in Classifier::GetLookupCache. r?gcp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36025/diff/1-2/
Attachment #8722428 - Attachment description: MozReview Request: Bug 1250439 - preven memory leak in Classifier::GetLookupCache. r?dietrich → MozReview Request: Bug 1250439 - preven memory leak in Classifier::GetLookupCache. r?gcp
Attachment #8722428 - Flags: review?(dietrich) → review?(gpascutto)
(Assignee)

Comment 6

3 years ago
I haven't used RefPtr because i don't think that we need a refcount smart ptr and also out object LookupCache didn't have implementation for AddRef and Release.
Comment on attachment 8722428 [details]
MozReview Request: Bug 1250439 - preven memory leak in Classifier::GetLookupCache. r?gcp

https://reviewboard.mozilla.org/r/36025/#review32845

::: toolkit/components/url-classifier/Classifier.cpp:695
(Diff revision 2)
> -  LookupCache *cache = new LookupCache(aTable, mStoreDirectory);
> +  nsAutoPtr<LookupCache> cache = new LookupCache(aTable, mStoreDirectory);

This doesn't compile.

0:04.78 /home/morbo/hg/mozilla-central/toolkit/components/url-classifier/Classifier.cpp: In member function 'mozilla::safebrowsing::LookupCache* mozilla::safebrowsing::Classifier::GetLookupCache(const nsACString_internal&)':
 0:04.78 /home/morbo/hg/mozilla-central/toolkit/components/url-classifier/Classifier.cpp:695:73: error: conversion from 'mozilla::safebrowsing::LookupCache*' to non-scalar type 'nsAutoPtr<mozilla::safebrowsing::LookupCache>' requested
 0:04.78    nsAutoPtr<LookupCache> cache = new LookupCache(aTable, mStoreDirectory);
 
In any case UniquePtr is the modern replacement for nsAutoPtr.
Attachment #8722428 - Flags: review?(gpascutto)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8722428 [details]
MozReview Request: Bug 1250439 - preven memory leak in Classifier::GetLookupCache. r?gcp

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36025/diff/2-3/
Attachment #8722428 - Flags: review?(gpascutto)
Attachment #8722428 - Flags: review?(gpascutto) → review+
Comment on attachment 8722428 [details]
MozReview Request: Bug 1250439 - preven memory leak in Classifier::GetLookupCache. r?gcp

https://reviewboard.mozilla.org/r/36025/#review32853

Thanks!

Comment 11

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