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

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Andi [:andi]]

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.

Comment 1

[Andi [:andi]]

Attachment: 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/

Comment 2

[Dietrich Ayala (:dietrich)]

GCP are you the right person to review this?

Comment 3

[Gian-Carlo Pascutto [:gcp]]

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?

Comment 4

[Andi [:andi]]

I'll update the patch and report it.

Comment 5

[Andi [:andi]]

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/

Comment 6

[Andi [:andi]]

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 7

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 8

[Andi [:andi]]

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/

Comment 9

[Gian-Carlo Pascutto [:gcp]]

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 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8441283083be

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8441283083be