281666 - Fixes unchecked use of strcpy/strcat + memory holes in nsDirPrefs.cpp

Status: VERIFIED FIXED

(MailNews Core :: Address Book, defect)

Description

[Tim Ruehsen]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.3; Linux) KHTML/3.3.2 (like Gecko)
Build Identifier: 

- found 2 or 3 places with memory not freed in any case (e.g. see 
dir_SaveReplicationInfo()) 
- introduced more NULL checks for function params 
- replaced strcpy/strcat by PR_snprintf+assertion on buffer overflow 
- removed these ugly and confusing 'scratch' buffers (this introduces a larger 
stack usage) 
 

Reproducible: Always

Comment 1

[Tim Ruehsen]

Attachment: cvs diff for nsDirPrefs.cpp

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 173865 [details] [diff] [review]
cvs diff for nsDirPrefs.cpp 

I'm setting r- on behalf of bienvenu.

This patch currently bitrots on apply, could you update it and post a fresh one? (and we'll try and review it a bit sooner this time).

Generally the idea looks good and it'll be nice to have a bit more of a tidy up in nsDirPrefs.

However whilst I'm here a couple of comments from a quick look at it:

+	if (!server)
+	{
+		NS_ASSERTION(PR_FALSE, "server is null");
+		return NS_ERROR_FAILURE;
+	}

a check for a null pointer passed to a function can be done better by NS_ENSURE_ARG_POINTER(server)

IMHO it would be better to convert the "scratch" buffer: 
-	PL_strcpy(scratch, prefRoot);
-	PL_strcat(scratch, ".");
-	PL_strcat(scratch, prefLeaf);

to nsAutoString or something similar rather than use PR_snprintf.

Comment 3

[Mark Banner (:standard8)]

I have checked with Tim, and he's no longer working on Mozilla :-( However, he said I could take his patch on and update it, so I'll do this at some stage in the next couple of weeks and tidy up nsDirPrefs a bit more.

Comment 4

[Mark Banner (:standard8)]

Attachment: New version (-w)

This takes Tim's original patch and reimplements it for the current tree. In addiiton, I've tried to make some improvements on the way, and also only checked for the presence of parameters where I think its really necessary (typically when the calling functions don't do it). I have also done a lot of tab removal and whitespace clean up - hence the -w patch.

Comment 5

[Mark Banner (:standard8)]

Attachment: New version (normal patch)

Comment 6

[[e-mail]]

Comment on attachment 204856 [details] [diff] [review]
New version (-w)

Mostly OK, but a couple of issues that apply in several places:

>+    nsXPIDLCString prefValue;
>+
>+    prefValue.Append(prefRoot);
>+    prefValue.Append('.');
>+    prefValue.Append(prefLeaf);
>  
>+    if (NS_SUCCEEDED(pPref->GetCharPref(prefValue, getter_Copies(value))))
This should be something like nsCAutoString prefLocation(prefRoot); and prefLocation.get();

>+    replPrefName.Append(NS_LITERAL_CSTRING(".replication"));
Don't Append an NS_LITERAL_CSTRING when you can AppendLiteral instead.

>+    if (!PR_snprintf (prefLocation, 128, "%s.filter%d", prefstring, filterNum))
>+    {
>+      NS_WARNING("DIR_GetCustomFilterPrefs: Buffer too small");
>+      continue;
>+    }
> 
>+    nsCAutoString branch(prefLocation);
Please construct an nsCAutoString prefLocation.

Comment 7

[Mark Banner (:standard8)]

Attachment: New version v2 (-w)

Revised version addressing Neil's review comments.

Comment 8

[Mark Banner (:standard8)]

Attachment: New version v2 (normal patch), checked in.

The non -w version.

Comment 9

[[e-mail]]

Comment on attachment 206917 [details] [diff] [review]
New version v2 (-w)

Some double blank lines strangely showed up on the interdiff, although there's obviously no sign of them here.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 206918 [details] [diff] [review]
New version v2 (normal patch), checked in.

This patch has now been checked in.
Checking in mailnews/addrbook/src/nsDirPrefs.cpp;
/cvsroot/mozilla/mailnews/addrbook/src/nsDirPrefs.cpp,v  <--  nsDirPrefs.cpp
new revision: 1.102; previous revision: 1.101

Comment 11

[Mark Banner (:standard8)]

This bug is now fixed, thanks to Tim for the initial patch.

Comment 12

[Mark Banner (:standard8)]

I just noticed I caused a regression in address book prefs - we are setting extra string prefs even when the values are the default values :-(

Comment 13

[Mark Banner (:standard8)]

Attachment: regression fix (checked in)

This patch fixes the comparison of the default value with the new value (not the prefLocation - opps). Think I probably overdid the copy & paste.

Comment 14

[Mark Banner (:standard8)]

Regression fix checked in -> fixed.

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified via proxy (reviews, code inspection using LXR, etc.)