289695 - nsFormHistory.cpp, nsAutoVoidArray should call PR_Free on matchingValues, and not delete it.

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Itay Perl]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050409 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050409 Firefox/1.0+

In mozilla\toolkit\components\satchel\src\nsFormHistory.cpp:
Line 790: delete (PRUnichar *) matchingValues[i];

but, while filling matchingValues with values, we actually call:
AppendElement->InsertElementAt:
nsVoidArray.cpp (InsertElementAt):
394   if (oldCount >= GetArraySize())
395   {
396     if (!GrowArrayBy(1))
397       return PR_FALSE;
398   }
Then we go to GrowArrayBy, which calls nsAutoVoidArray::SizeTo, which
calls...nsVoidArray::SizeTo

in nsVoidArray::SizeTo we have:
186     char* bytes = (char *) PR_Realloc(mImpl,SIZEOF_IMPL(aSize));
-or-
214     char* bytes = (char *) PR_Malloc(SIZEOF_IMPL(aSize));

Which uses malloc and realloc, which come from the malloc family after all.

AFAIK, this is a very easy fix: just change delete matchingValues[i];, to
free(matchingValues[i]);

Reproducible: Always

Steps to Reproduce:




Leak detected with BoundsChecker Version 7.2.0

Comment 1

[Itay Perl]

A little correction:

AFAIK, this is a very easy fix: just change delete (PRUnichar *)
matchingValues[i];, to
free((PRUnichar *)matchingValues[i]); (CMIIW)

Comment 2

[Justin Wood (:Callek)]

[quote]Memory allocated by PR_Malloc, PR_Calloc, or PR_Realloc must be freed by
PR_Free.[/quote] (Source:
http://www.mozilla.org/projects/nspr/reference/html/prmem2.html)

Matching Value's Used at:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/satchel/src/nsFormHistory.cpp&rev=1.24&mark=755,768,782,783,790#754

And actually the bug is not that the wrong free is called, it is that the free
is done here, nsAutoVoidArray Frees its allocated values on destruction.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpcom/ds/nsVoidArray.cpp&rev=3.41&mark=357,361#357

(Note no Allocation in NS_QuickSort)

Comment 3

[Justin Wood (:Callek)]

Attachment: remove free

Fix as I understand it.

Comment 4

[Itay Perl]

I guess you're right, but still I'm kinda sure something is wrong here.
I got my mistake. I looked for the wrong thing. I found the real bug (I hope).
the problematic line, as I said, is: delete (PRUnichar *) matchingValues[i];
which is the variable in the array, not the array itself. See this:
in line 766 (mozilla\toolkit\components\satchel\src\nsFormHistory.cpp):
if (RowMatch(row, aInputName, aInputValue, &value)) {
so I went to nsFormHistory::RowMatch.
Look at line 834 of mozilla/toolkit/components/satchel/src/nsFormHistory.cpp, we
have:
GetRowValue(aRow, kToken_ValueColumn, value); 
so I went to nsFormHistory::GetRowValue. 
OK?
OK!
now nsFormHistory::GetRowValue's aValue is a nsAString, which is, depends on
I-dunno-what, a nsTAString_CharT or nsTSubstring_CharT
(http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/string-template-def-unichar.h).
now look. in nsFormHistory::GetRowValue, we have this (line 707):
aValue.Assign((const PRUnichar *)yarn.mYarn_Buf,
yarn.mYarn_Fill/sizeof(PRUnichar)); 

nsTAString_CharT::Assign( const char_type* data, size_type length )
(http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsTAString.cpp#257)
Is calling nsTSubstring_CharT::Assign (I'm leaving the Obselete one for now)

nsTSubstring_CharT::Assign((http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsTSubstring.cpp#287)
which calls nsTSubstring_CharT::ReplacePrep in line 306.

nsTSubstring_CharT::ReplacePrep is calling nsTSubString_CharT::MutatePrep
(http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsTSubstring.cpp#189)

in nsTSubString::MutatePrep we have a few cases, but you can see that for
allocating we are calling nsStringBuffer::Alloc or nsStringBuffer::Realloc.

nsStringBuffer::Alloc calls malloc (btw, Why doesn't it call PR_Malloc?) here:
http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsSubstring.cpp#204
and nsStringBuffer::Realloc calls realloc (Why not PR_Realloc?) here:
http://lxr.mozilla.org/seamonkey/source/xpcom/string/src/nsSubstring.cpp#223

that's the right explanation to the bug. sorry for the mistake, and I hope this
one is true.

Comment 5

[Itay Perl]

My solution is to change delete (PRUnichar *) matchingValues[i];
to free((PRUnichar *) matchingValues[i]); or something equivalent

Comment 6

[Itay Perl]

Attachment: Delete to PR_Free [checked in]

It's even shorter than I thought. 
That's the last time I'll nag.

That's how it happens:

toolkit/components/satchel/src/nsFormHistory.cpp#766: RowMatch(row, aInputName,
aInputValue, &value)

[Same file]#827, in RowMatch: *aValue = ToNewUnicode(value). 
This means our Value (which his freeing is the matter of this bug), will be
ToNewUnicode(value)'s return value. simple.

xpcom/string/src/nsReadableUtils.cpp#375: PRUnichar* result =
AllocateStringCopy(asource, (PRUnichar*)0);

xpcom/string/src/nsReadableUtils.cpp#310: we allocate memory using
nsMemory::Alloc

nsMemory::Alloc in
xpcom\glue\nsmemory.h#68: { return NS_Alloc(size); }

xpcom/base/nsMemoryImpl.cpp#363:
NS_Alloc calls MALLOC1 which is a macro that calls (directly or through
mallocator) to PR_Malloc.

http://lxr.mozilla.org/seamonkey/source/toolkit/components/satchel/src/nsFormHistory.cpp#790:
here we delete it instead of PR_Freeing it.

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 180351 [details] [diff] [review]
remove free

I think I was wrong.

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 182271 [details] [diff] [review]
Delete to PR_Free [checked in]

Please hold off on requesting approval until you've got the required reviews
and can provide a specific request to drivers  that includes justification for
taking the patch during the freeze. Thanks.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

The patch is wrong. ToNewUnicode uses nsMemory as allocator, so it must be freed
using nsMemory too (or NS_Free, which is the same).

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 182271 [details] [diff] [review]
Delete to PR_Free [checked in]

This patch was checked in by Daniel Brooks.

Comment 11

[Itay Perl]

Attachment: Patch 2

Well...if it's important...

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

is the cast required?

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 186304 [details] [diff] [review]
Patch 2

flipping to bsmedberg, since my C++ mojo is obviously not strong enough in this
case.

Comment 14

[Itay Perl]

well, it compiles without the cast, but I don't know, it may change
something...reviewer (benjamin?), please tell me if it's needed.

thanks

Comment 15

[Benjamin Smedberg]

Comment on attachment 186304 [details] [diff] [review]
Patch 2

1) please use NS_Free in all new code and

2) the cast is unneeded, please remove

Comment 16

[Itay Perl]

Attachment: Patch 2b [checked in]

Comment 17

[Benjamin Smedberg]

Comment on attachment 186473 [details] [diff] [review]
Patch 2b [checked in]

r=me, but please make sure to use spaces instead of a tab before getting this
checked in

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in toolkit/components/satchel/src/nsFormHistory.cpp;
/cvsroot/mozilla/toolkit/components/satchel/src/nsFormHistory.cpp,v  <-- 
nsFormHistory.cpp
new revision: 1.26; previous revision: 1.25
done