Closed Bug 1250439 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 749516)

Attachments

(1 file)

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.
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
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)
I'll update the patch and report it.
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)
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)
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!
https://hg.mozilla.org/mozilla-central/rev/8441283083be
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.