173634 - flawfinder warnings in xp_str.h: kill xp_str.h, xp_path.h and clean out old/dead defines/code

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Stephen P. Morse]

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

flawfinder found 1 warning in xp_str.h code (1832). 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.

1832) include/xp_str.h:66 [4] (buffer) strcpy: does not check for buffer 
overflows. Consider using strncpy or strlcpy.

Comment 1

[timeless]

bogus. xp_str.h is used by very few people (and should be killed)

http://lxr.mozilla.org/seamonkey/ident?i=XP_STRCPY

XP_STRCPY
Defined as a preprocessor macro in:
A include/xp_str.h, line 66
B modules/libreg/src/vr_stubs.h, line 103
B modules/libreg/src/vr_stubs.h, line 204

Referenced (in 8 files total) in:
! db/mork/src/morkConfig.h -- looks dead
* include/xp_str.h -- file in question
B modules/libreg/src/VerReg.c
B modules/libreg/src/vr_stubs.c
B modules/libreg/src/reg.c
* modules/libreg/src/vr_stubs.h -- other file that defines it
! security/nss/lib/util/secport.h -- ifdef gone
(http://lxr.mozilla.org/mozilla/search?string=XP_STRING_FUNCS)
! mailnews/compose/src/nsMsgAppleDoubleDecode.cpp -- is dead
http://lxr.mozilla.org/seamonkey/search?string=nsMsgAppleDoubleDecode.cpp

oh, and of course it's only a define, flawfinder needs to be smart enough to
figure that out and check the consumers instead...

Comment 2

[timeless]

Attachment: kill them all

Removed Files:
	mozilla/include/xp_path.h mozilla/include/xp_str.h 
	mozilla/mailnews/compose/src/nsMsgAppleDecode.cpp 
	mozilla/mailnews/compose/src/nsMsgAppleDecodeStream.cpp 
	mozilla/mailnews/compose/src/nsMsgAppleDoubleDecode.cpp 

other removed defines:
UNREADY_CODE

Comment 3

[timeless]

Here's a list of Modules, Files, and Owners.
If the owners or mozilla.org would sign off on this then I'll work on getting it
in at some point (depending on how important it is to mozilla.org).

DOM <[e-mail]> {Forms} <[e-mail]>
mozilla/content/html/content/src/nsFormSubmission.cpp

MathML <[e-mail]>
mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp

News <[e-mail]>
mozilla/mailnews/news/src/nsNNTPProtocol.cpp

OJI <[e-mail],[e-mail]>
mozilla/modules/oji/src/nsJVMManager.cpp

NSS <[e-mail],[e-mail],[e-mail]>
mozilla/security/nss/cmd/lib/filestub.c
mozilla/security/nss/cmd/lib/sslstubs.c
mozilla/security/nss/lib/freebl/mac_rand.c
mozilla/security/nss/lib/jar/jarint.h
mozilla/security/nss/lib/util/secport.h

XPApps <[e-mail]> {Internet Search} <[e-mail]>
mozilla/xpfe/components/search/src/nsInternetSearchService.cpp

[owners.html didn't know] <[e-mail]> {Mork} <[e-mail]>
mozilla/db/mork/src/morkConfig.h <[e-mail]>

[owners.html didn't know] <[e-mail]>
mozilla/include/MANIFEST <[e-mail]>
mozilla/include/Makefile.in <[e-mail]>
mozilla/mailnews/addrbook/src/nsDirPrefs.cpp <[e-mail]>
mozilla/mailnews/addrbook/src/nsDirPrefs.h <[e-mail]>
mozilla/mailnews/base/search/src/nsMsgSearchNews.cpp <[e-mail]>
mozilla/mailnews/compose/src/nsSmtpProtocol.cpp <[e-mail]>
mozilla/mailnews/imap/src/nsImapProtocol.cpp <[e-mail]>
mozilla/mailnews/local/src/nsPop3Protocol.cpp <[e-mail]>

Comment 4

[Stephen P. Morse]

Please don't remove the orginal subject line.  We need to be able to track the
flawfinder results by doing a search on "flawfinder"

Comment 5

[timeless]

you really don't, this isn't a flawfinder bug, it's my excuse for killing the
file which the broken flawfinder happened to flag. Besides you have bug 148521
to track flawfinder. 

Comment 6

[timeless]

here's the list of consumers:
http://lxr.mozilla.org/seamonkey/search?string=%22xp_str.h%22

"xp_str.h"
/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp, line 35 -- #include "xp_str.h"
// to get XP_IS_SPACE
/mailnews/addrbook/src/nsDirPrefs.cpp, line 57 -- #include "xp_str.h"

The first is changed to use a different file "nsTextFragment.h"
The second was a NOOP because XP_PlatformFileToURL was not defined and the patch
removed it. Really flawfinder didn't find flaws. I found unused/unreachable code.

Comment 7

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

see bug 171883

Comment 8

[rbs]

Index: mozilla/layout/mathml/base/src/nsMathMLmpaddedFrame.cpp
===================================================================

-#include "xp_str.h"  // to get XP_IS_SPACE
+#include "nsTextFragment.h"

nsTextFragment has its own purpose that has nothing to do with
nsMathMLmpaddedFrame. Son your change can cause confusion. You might want to
instead put XP_IS_SPACE in nsUnicharUtils.h which is where ToUpperCase(),
ToLowerCase(), etc, come from.

Comment 9

[rbs]

s/Son your change can cause confusion/So your change can cause confusion/

Comment 10

[Brian Ryner (not reading)]

You'll need to get separate review from the NSS team for the NSS code changes
(I'd be surprised if NSS used mozilla/include/ anyway).  You should also get
someone on mailnews to review the code removal there, although it all looks good
to me.

Comment 11

[Stephen P. Morse]

1 more flawfinder bug in xp_str.h (4492)

4492) include/xp_str.h:66 [4] (buffer) strcpy: does not check for buffer
overflows. Consider using strncpy or strlcpy. 

Comment 12

[timeless]

irrelevant, i'm killing the file.

Comment 13

[QA timeless]

rbs: that'd be an interesting endeavor, however of the two definitions of
XP_IS_SPACE in cvs, neither are intl safe or friendly.

here's the one i'm killing:
#define XP_IS_SPACE(VAL)                \
    (((((intn)(VAL)) & 0x7f) == ((intn)(VAL))) && isspace((intn)(VAL)) )

according to a linux man page:

isspace()
checks for white-space characters. In the "C" and "POSIX" locales, these are:
space, form-feed ('\f'), newline ('\n'), carriage return ('\r'), horizontal tab
 ('\t'), and vertical tab ('\v').

here's the one i tried to make your code use:

// XXX these need I18N spankage
#define XP_IS_SPACE(_ch) \
  (((_ch) == ' ') || ((_ch) == '\t') || ((_ch) == '\n'))

Any intl owner would be well justified in killing me with a dull spoon for
trying to introduce either of these definitions into an intl file.

How about for the time being I just move the one i'm killing directly into your
file?

Comment 14

[rbs]

The #define XP_IS_SPACE() that you are trying to remove looks OK to me for
general use. Why isn't it intl safe, BTW? It seems to be using a conservative
test that avoids troubles. (ToLowerCase/ToUpperCase are problematic too if not
implemented with similar care. And they are in nsUnicharUtils.h)

Comment 15

[timeless]

well, there's ‌ for one, i don't know the other unicode codepoints which
qualify as spaces, but i'm sure smontagu or hixie could list them.

plus while the macro you're looking at works on utf8 and ucs2, a macro to cover
unicode space characters probably wouldn't work since their representations are
rather different.

Comment 16

[rbs]

Actually, just replace with nsCRT::IsAsciiSpace() -- which does what I wanted.
With that, the i18n problem boils to nsCRT::IsAsciiSpace().

Comment 17

[Alec Flett]

Comment on attachment 102453 [details] [diff] [review]
kill them all

good riddens! sr=alecf

Comment 18

[Simon Montagu :smontagu]

It depends what you mean by "space". According to
http://www.unicode.org/Public/UNIDATA/PropList.txt, 24 Unicode characters have
the White_Space property, which means that they should be treated by programming
languages as "white space" for the purpose of parsing. Is that what is required
here?

0009..000D    ; White_Space # Cc   [5] <control>..<control>
0020          ; White_Space # Zs       SPACE
0085          ; White_Space # Cc       <control>
00A0          ; White_Space # Zs       NO-BREAK SPACE
1680          ; White_Space # Zs       OGHAM SPACE MARK
2000..200A    ; White_Space # Zs  [11] EN QUAD..HAIR SPACE
2028          ; White_Space # Zl       LINE SEPARATOR
2029          ; White_Space # Zp       PARAGRAPH SEPARATOR
202F          ; White_Space # Zs       NARROW NO-BREAK SPACE
3000          ; White_Space # Zs       IDEOGRAPHIC SPACE

Comment 19

[rbs]

Indeed, "space" is a context sensitive issue. That list is overkill and might
cause other problems of its own (re: web context). What was required was ascii
space (or, perhaps, to put it a safer way: "collapsable space"). So things such
as nbsp, thinsp, emsp, etc, are better kept out of this (re: web context).

Comment 20

[David :Bienvenu]

mail stuff looks OK.

Comment 21

[Simon Montagu :smontagu]

I just came across the CSS definition of "whitespace" today, so am adding it
here in case it's relevant:

'Only the characters "space" (Unicode code 32), "tab" (9), "line feed" (10),
"carriage return" (13), and "form feed" (12) can occur in whitespace. Other
space-like characters, such as "em-space" (8195) and "ideographic space"
(12288), are never part of whitespace.'

http://www.w3.org/TR/REC-CSS2/syndata.html#whitespace

Comment 22

[timeless]

*** Bug 171879 has been marked as a duplicate of this bug. ***

Comment 23

[Mike Kaply [:mkaply]]

Comment on attachment 102453 [details] [diff] [review]
kill them all

r=mkaply for the removal of the Mac files - let's get this bug off of the
books.

Comment 24

[timeless]

Comment on attachment 102453 [details] [diff] [review]
kill them all

mozilla/mailnews/compose/src/Attic/nsMsgAppleDecode.cpp 	1.12
mozilla/mailnews/compose/src/Attic/nsMsgAppleDecodeStream.cpp	1.9
mozilla/mailnews/compose/src/Attic/nsMsgAppleDoubleDecode.cpp	1.5
mozilla/mailnews/compose/src/nsSmtpProtocol.cpp 	1.176
mozilla/mailnews/base/search/src/nsMsgSearchNews.cpp	1.36
mozilla/mailnews/news/src/nsNNTPProtocol.cpp	1.366
mozilla/db/mork/src/morkConfig.h	1.26
mozilla/xpfe/components/search/src/nsInternetSearchService.cpp	1.229