237190 - Migration Leaks

Status: RESOLVED FIXED

(Firefox :: Migration, defect, P3)

Description

[Ben Goodger (use ben at mozilla dot org for email)]

A tracking bug for memory leaks in Migration code.

Comment 1

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: fix some leaks

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 143669 [details] [diff] [review]
fix some leaks

OK - I found the following leaks with purify:
- callers of KeyIsURI are leaking realm
- nsINIParser leaks entire file buffer
- profile migrators using MIGRATIONDATA array are leaking file name strings
associated with entries that are skipped by aReplace/replaceOnly condition. 

In the process of fixing #3, reorganized code into a shared utility function
that does not have a hard-coded iteration length for more security from
copy-paste errors and future additions

Comment 3

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: one that works ;-)

pass the length of the data array to the helper function as well, so we know
where the end of the array is.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 143673 [details] [diff] [review]
one that works ;-) 

>+#include "nsBrowserProfileMigratorUtils.h"
>+#include "nsILocalFile.h"
>+#include "nsCRT.h"
>+
>+void SetProxyPref(const nsACString& aHostPort, const char* aPref, 
>+                  const char* aPortPref, nsIPrefBranch* aPrefs) 
>+{
>+  nsCAutoString hostPort(aHostPort);  
>+  PRInt32 portDelimOffset = hostPort.RFindChar(':');
>+  if (portDelimOffset) {
>+    nsCAutoString host(Substring(hostPort, 0, portDelimOffset));    
>+    nsCAutoString port(Substring(hostPort, portDelimOffset + 1, hostPort.Length() - portDelimOffset + 1));
>+    
>+    aPrefs->SetCharPref(aPref, host.get());
>+    PRInt32 stringErr;
>+    PRInt32 portValue = port.ToInteger(&stringErr);
>+    aPrefs->SetIntPref(aPortPref, portValue);
>+  }
>+  else {
>+    aPrefs->SetCharPref(aPref, hostPort.get());
>+  }
>+}

Can you get darin or dbaron to comment on the string-fu here?  Or tell me you
do, cuz .get() doens't work on nsACString (I thought it did, since darin landed
his get-rid-of-multi-fragment-strings changes).  Or do you need to copy into
nsCAutoString hostPort in order to ensure nul termination?

In which case, once you have hostPort, can't you use const nsACString &host and
&port refs to the Substrings, as you do in the next function, instead of
copying into yet more nsCAutoStrings?

>+void ParseOverrideServers(const char* aServers, nsIPrefBranch* aBranch)
>+{
>+  // First check to see if the value is "<local>", if so, set the field to 
>+  // "localhost,127.0.0.1"
>+  nsCAutoString override; override = aServers;

Why not pass aServers to the ctor as you did in the previous function?

Also, why unroll the loop one iteration here, with a special case for
"<local>"?  The else part will do the right thing for that input.

>+  if (override.Equals("<local>")) {
>+    aBranch->SetCharPref("network.proxy.no_proxies_on", "localhost,127.0.0.1");
>+  }
>+  else {
>+    // If it's not, then replace every ";" character with ","
>+    PRInt32 offset = 0, temp = 0;
>+    while (offset != -1) {

Nit: offset is 0 on entry, so use a do-while loop, not a while loop.

>+      offset = override.FindChar(';', offset);
>+      const nsACString& host = Substring(override, temp, offset < 0 ? override.Length() - temp : offset - temp);
>+      if (host.Equals("<local>"))
>+        override.Replace(temp, 7, NS_LITERAL_CSTRING("localhost,127.0.0.1"));
>+      temp = offset + 1;
>+      if (offset != -1)

Nit^2: break here if offset == -1, use a while (true) or for (;;) loop.  But
see next comment.

>+        override.Replace(offset, 1, NS_LITERAL_CSTRING(","));
>+    }
>+    aBranch->SetCharPref("network.proxy.no_proxies_on", override.get());
>+  }

You can simplify this whole function body as follows:

  nsCAutoString override(aServers);
  PRInt32 offset = 0, start = 0;
  for (;;) {
    offset = override.FindChar(';', offset);
    const nsACString& host = Substring(override, start, (offset < 0 ?
override.Length() : offset) - start);
    if (host.Equals("<local>"))
      override.Replace(start, 7, NS_LITERAL_CSTRING("localhost,127.0.0.1"));
    if (offset < 0)
      break;
    start = offset + 1;
    override.Replace(offset, 1, NS_LITERAL_CSTRING(","));
  }
  aBranch->SetCharPref("network.proxy.no_proxies_on", override.get());

>+void GetMigrateDataFromArray(MIGRATIONDATA* aDataArray, PRInt32 aDataArrayLength, 
>+                             PRBool aReplace, nsILocalFile* aSourceProfile, 
>+                             PRUint16* aResult)
>+{
>+  nsCOMPtr<nsIFile> sourceFile; 
>+  PRBool exists;
>+  MIGRATIONDATA* cursor;
>+  MIGRATIONDATA* end = aDataArray + aDataArrayLength;
>+  for (cursor = aDataArray; cursor < end; ++cursor) {
>+    // Don't list items that can only be imported in replace-mode when
>+    // we aren't being run in replace-mode.
>+    if (!aReplace && cursor->replaceOnly)  {
>+      nsCRT::free(cursor->fileName);

Null cursor->fileName.	Even if no one uses it after the calls to this
function, why leave a dangling pointer for some later hacker to trip over?

>+      continue;

Since you nsCRT::free cursor->fileName in two places due to this continue,
maybe this is a case where testing

    if (aReplace || cursor->replaceOnly) {
      clone and append and check existence to set sourceFlag
    }
    nsCRT::free

is better than using continue.	The if condition is clearer to me, at any rate.

>+    }
>+
>+    aSourceProfile->Clone(getter_AddRefs(sourceFile));
>+    sourceFile->Append(nsDependentString(cursor->fileName));

Check for Append failure, because if sourceFile refers to the profile after an
OOM error, you'll test the existence of the source profile and set the flag
wrongly.

>+    sourceFile->Exists(&exists);
>+    if (exists)
>+      *aResult |= cursor->sourceFlag;
>+
>+    nsCRT::free(cursor->fileName);

Null it.  Better yet, eliminate the continue and null only once (here, in that
case).

>+  }
>+}

> typedef struct { 
>   PRUnichar* fileName; 
>   PRUint32 sourceFlag;
>   PRBool replaceOnly;
> } MIGRATIONDATA;

Nit: This is old-school (old Unix or Windows typedef naming style) and C-like
-- in C++, you can just say struct FOO {...} and use FOO as a typename. What's
more, in Mozilla, usually the typename is InterCaps or prefixInterCaps.  In
that light, how about

struct MigrationData {
  PRUnichar* fileName; 
  PRUint32 sourceFlag;
  PRBool replaceOnly;
};

here?

>+  // Frees file name strings allocated above.
>+  GetMigrateDataFromArray((MIGRATIONDATA*)data, 

Why is the cast needed?  Isn't data declared just above to be of type
MIGRATIONDATA []?  Array names used in expressions become pointers to the first
element, so there shoudn't be any need to cast.

>+++ migration/src/nsIEProfileMigrator.cpp	12 Mar 2004 00:55:14 -0000
>@@ -794,16 +794,20 @@ nsIEProfileMigrator::GetSignonsListFromP
>               // phase is complete do we free the buffer. 
>               SIGNONDATA* d = new SIGNONDATA;

Hmph, more UGLYTYPEDEFNAMES.  Is this based on some old morse code?

>               d->user = (PRUnichar*)username;
>               d->pass = (PRUnichar*)pass;
>               d->realm = realm;
>               aSignonsFound->AppendElement(d);
>             }
>           }
>+          if (realm) {
>+            nsCRT::free(realm);
>+            realm = nsnull;
>+          }

Why not use nsXPIDLCString realm, as you do in later files?

Mixing manual and automagic storage management is generally a bad idea, unless
there's some hard case or optimization at stake.

Mix of four-space and two-space indentation across the files in this patch --
shouldn't they all use the same indentation offset?

/be

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: again

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Some notes - 

.get() does not work on the const nsACString& returned by Substring(). I
replaced the copy with nsDependentCStrings. 

I assume by 2/4 indentation you are referring to nsINIParser. I did not write
that file, merely copied it over from xpinstall. I left its indentation in tact. 

I actually find if (aReplace || !cursor->replaceOnly) more difficult to
understand as it addresses two cases rather than just one - but it simplifies
the cleanup so I'm using it anyway, and enhancing the comment. 

Oh and as for all the typedef struct { } TYPENAME ... this is what you get for
working too closely with WinAPI code :-P This newer patch replaces them with
struct TypeName { }

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: forgot about last comment

switch to nsXPIDLCStrings for the last KeyIsURI call too, and copy the result
into the SignonData entry being returned. This string is freed in
ResolveAndMigrateSignons...

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: actually, why am I bothering copying?

mexican jumping patches.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 143747 [details] [diff] [review]
actually, why am I bothering copying?

>+void SetProxyPref(const nsACString& aHostPort, const char* aPref, 
>+                  const char* aPortPref, nsIPrefBranch* aPrefs) 
>+{
>+  nsCAutoString hostPort(aHostPort);  
>+  PRInt32 portDelimOffset = hostPort.RFindChar(':');
>+  if (portDelimOffset) {
>+    nsDependentCString host(Substring(hostPort, 0, portDelimOffset));
>+    nsDependentCString port(Substring(hostPort, portDelimOffset + 1, hostPort.Length() - portDelimOffset + 1));

Bug (dunno whether Substring clamps its third arg against the intrinsic length
of the first arg):

    nsDependentCString port(Substring(hostPort, portDelimOffset + 1,
			    hostPort.Length() - (portDelimOffset + 1)));

>+void ParseOverrideServers(const char* aServers, nsIPrefBranch* aBranch)
>+{
>+  // Windows (and Opera) formats its proxy override list in the form:
>+  // server;server;server where server is a server name or ip address, 
>+  // or "<local>". Mozilla's format is server;server;server, and <local>
>+  // must be translated to "localhost,127.0.0.1"
>+  nsCAutoString override(aServers);
>+  PRInt32 left = 0, right = 0;

Nice, better names!

>+  for (;;) {
>+    right = override.FindChar(';', right);
>+    const nsACString& host = Substring(override, left, 
>+                                       (right < 0 ? override.Length() : right) - left);
>+    if (host.Equals("<local>"))
>+      override.Replace(left, 7, NS_LITERAL_CSTRING("localhost,127.0.0.1"));
>+    if (left < 0)

Bug: if (right < 0).

>+      break;
>+    left = right + 1;
>+    override.Replace(right, 1, NS_LITERAL_CSTRING(","));
>+  }
>+  aBranch->SetCharPref("network.proxy.no_proxies_on", override.get());
>+}
>+
>+void GetMigrateDataFromArray(MigrationData* aDataArray, PRInt32 aDataArrayLength, 
>+                             PRBool aReplace, nsILocalFile* aSourceProfile, 
>+                             PRUint16* aResult)
>+{
>+  nsCOMPtr<nsIFile> sourceFile; 
>+  PRBool exists;
>+  MigrationData* cursor;
>+  MigrationData* end = aDataArray + aDataArrayLength;
>+  for (cursor = aDataArray; cursor < end; ++cursor) {
>+    if (cursor->fileName) {

Here is where if (!cursor->fileName) continue; would pay off -- no duplicated
code, no overindented normal-case code in the loop body.

> typedef struct {
>   PRUnichar* user;
>   PRUnichar* pass;
>   char*      realm;
>-} SIGNONDATA;
>+} SignonData;

struct SignonData {...};

>+++ src/nsNetscapeProfileMigratorBase.h	12 Mar 2004 22:20:51 -0000
>@@ -60,17 +60,17 @@ public:
>     prefConverter prefGetterFunc;
>     prefConverter prefSetterFunc;
>     PRBool        prefHasValue;
>     union {
>       PRInt32     intValue;
>       PRBool      boolValue;
>       char*       stringValue;
>     };
>-  } PREFTRANSFORM;
>+  } PrefTransform;

More typedef C madness -- use C++ when in C++.	Same for other occurrences
below.

/be

Comment 10

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: some more tidy up

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 143774 [details] [diff] [review]
some more tidy up

Might as well give up in my quest for out of memory checking. It adds code that
will run only on platforms without the modern, dreaded style of "overcommit VM
and then kill processes after thrashing to death" OS virtual memory
implementation.