173992 - flawfinder warnings in Mailnews Address Book

Status: RESOLVED WORKSFORME

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[Stephen P. Morse]

Heikki ran flawfinder (http://www.dwheeler.com/flawfinder) on Mozilla 1.0.1 
branch.

flawfinder found 2 warnings in address book code (3727). Go through
that list and for each warning:

* If it is false positive, comment here why it is not an issue
* If it is a real issue, make patch for it here and let's get them checked in

In addition to checking the branch, also check the trunk.

3727) mailnews/addrbook/src/nsAddrDatabase.cpp:484 [4] (buffer) strcpy: does not 
check for buffer overflows. Consider using strncpy or strlcpy.

Comment 1

[Stephen P. Morse]

Correction: I meant to say that he found 1 warning, not 2.

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This code uses strcpy safely, but I suspect there is a mismatched new/free
lurking here. In the beginning of the function nsAddrDatabase::UnixToNative() we
dynamically allocate with |new|, but at the end of the function we release with
nsCRT:free(), which calls |free()| I believe. We allocate/release different
variables inside this single function call, but if somebody calls this function
with a value that was received from this function we have a problem, and of
course we would have a problem if someone called this function with any value
that was allocated with |new|.

Comment 3

[Stephen P. Morse]

Note also that the routine involved with this warning (nsAddrDatabase.cpp:484)
is nearly identical to the routine involved with warning 3741 in bug 173997
(nsMsgDatabase.cpp:894), and is therefore safe for the same reasons as cited
there.  The only differerence between these two routines is that one uses
PL_strchr and the other uses just strchr.  These should be factored into a
common routine so as to reduce footprint and also to avoid divergence such as
already has occurred.

Comment 4

[(not reading, please use [e-mail] instead)]

mass re-assign.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Closing all open flawfinder bugs as WORKSFORME because we now have much better tools that do the same (well, better) kind of analysis (Coverity, Klocwork).