Last Comment Bug 298612 - Audit consumers of isspace
: Audit consumers of isspace
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla1.9alpha1
Assigned To: Wan-Teh Chang
: Yuying Long
Mentors:
Depends on: 290182
Blocks:
  Show dependency treegraph
 
Reported: 2005-06-23 12:25 PDT by timeless
Modified: 2005-08-15 15:09 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
nss part (3.39 KB, patch)
2005-07-08 15:41 PDT, Bastiaan Jacques
no flags Details | Diff | Splinter Review
nspr part (2.14 KB, patch)
2005-07-08 15:45 PDT, Bastiaan Jacques
no flags Details | Diff | Splinter Review
nss part (compiles) (3.98 KB, patch)
2005-07-12 13:23 PDT, Bastiaan Jacques
wtc: review-
Details | Diff | Splinter Review
nspr part (compiles) (2.60 KB, patch)
2005-07-12 13:25 PDT, Bastiaan Jacques
no flags Details | Diff | Splinter Review
nss part, v2 (checked in) (750 bytes, patch)
2005-07-13 16:02 PDT, Wan-Teh Chang
nelson: review+
Details | Diff | Splinter Review
nspr part, v2 (checked in) (519 bytes, patch)
2005-07-13 16:12 PDT, Wan-Teh Chang
darin.moz: review+
Details | Diff | Splinter Review

Description timeless 2005-06-23 12:25:36 PDT
per bug 290182 comment 9 and review comments from smontagu, we want to make sure
all instances of isspace in our tree don't pass high bits of chars or random
ints to isspace (and instead either use NS_IS_SPACE or iswspace).
Comment 1 Bastiaan Jacques 2005-06-23 13:50:01 PDT
My initial thoughts: checking for 0 <= char <= 255 every time would be a painful
and repetitive process. Replacing all our usage of isspace() with NS_IS_SPACE()
seems to be a safe way to proceed, albeit slightly slower than the current
situation.
Comment 2 Bastiaan Jacques 2005-06-26 16:01:53 PDT
smontagu, would you care to share your thoughts on the matter with us? timeless
led me to believe you might have an opinion on this bug.
Comment 3 Simon Montagu :smontagu 2005-06-27 01:55:12 PDT
I agree with comment 1, assuming that by "all our usage" you are only referring
to usage with PRUnichar or equivalent.
Comment 4 Bastiaan Jacques 2005-06-27 05:25:02 PDT
Actually, I meant all our usage. It is possible for chars as well as PRUnichars
to be outside of the range of EOF or 0 through 0xFF, which would cause undefined
behaviour, on win32. (for glibc, the behaviour is undefined when the argument is
not EOF or an unsigned char)
Comment 5 Bastiaan Jacques 2005-06-27 05:49:18 PDT
..Actually, while discussing this on IRC it turned out my test code was wrong
and a (signed or unsigned) char is safe to use with isspace().

Comment 6 Bastiaan Jacques 2005-07-07 16:34:18 PDT
Note to self:

The following uses of isspace() require additional investigation.

http://lxr.mozilla.org/seamonkey/source/db/sqlite3/src/build.c#950
(more in sqlite)

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/src/io/prscanf.c#432
(more in prscanf.c)

http://lxr.mozilla.org/seamonkey/source/security/nss/cmd/signtool/javascript.c#428
(maybe? other occurrances)
Comment 7 Bastiaan Jacques 2005-07-08 15:41:39 PDT
Created attachment 188730 [details] [diff] [review]
nss part
Comment 8 Bastiaan Jacques 2005-07-08 15:45:02 PDT
Created attachment 188731 [details] [diff] [review]
nspr part

I assume the sqlite changes will have to be done by the sqlite project itself.
Comment 9 Bastiaan Jacques 2005-07-12 13:23:27 PDT
Created attachment 189088 [details] [diff] [review]
nss part (compiles)
Comment 10 Bastiaan Jacques 2005-07-12 13:25:33 PDT
Created attachment 189089 [details] [diff] [review]
nspr part (compiles)
Comment 11 Wan-Teh Chang 2005-07-13 13:46:00 PDT
Related macros like isdigit and isalpha have the
same problem, right?
Comment 12 Wan-Teh Chang 2005-07-13 13:56:29 PDT
Comment on attachment 189088 [details] [diff] [review]
nss part (compiles)

This patch is either not necessary or not the
best way to fix the problem.

The variable "curchar" has the "int" type and
its value is promoted from a "char" in FB_GetChar().

If this char-to-int promotion may result in a
value outside 0 to 0xFF, it is best to fix this
by doing a char-to-unsigned char cast in
FB_GetChar().
Comment 13 Wan-Teh Chang 2005-07-13 16:02:47 PDT
Created attachment 189236 [details] [diff] [review]
nss part, v2 (checked in)

This is a better way to fix the problem because
it also takes care of the other isxxx macros.

The "unsigned char" cast in FB_GetChar() is
inspired by the getchar() sample code on pages
171-172 of The C Programming Language, 2nd. Ed.
It "eliminates any problem of sign extension."
Comment 14 Wan-Teh Chang 2005-07-13 16:12:45 PDT
Created attachment 189238 [details] [diff] [review]
nspr part, v2 (checked in)

This is a better way to fix this problem in NSPR.

The "unsigned char" cast is inspired by the getchar()
sample code on page 172 of The C Programming Language,
2nd. Ed.  It "eliminates any problem of sign extension."
(p. 171)
Comment 15 Bastiaan Jacques 2005-07-15 05:41:11 PDT
(In reply to comment #12)
> If this char-to-int promotion may result in a
> value outside 0 to 0xFF, it is best to fix this
> by doing a char-to-unsigned char cast in
> FB_GetChar().

I don't think isspace() cares about signedness of chars. All it requires is that
the argument passed is no larger than (unsigned) 255 (in other words, signed
char -20 is an acceptable value). With your patch, FB_GetChar() still returns
(int) EOF, which results in the value of (unsigned) 4294967295 being passed to
isspace().

Casting the return value of FB_GetChar would still be an acceptable solution,
but you'd have to cast EOF, too. For example:

return (char)retval;
Comment 16 Wan-Teh Chang 2005-07-15 09:50:31 PDT
We need to agree on what the problem is, otherwise we can't
agree on what the right solution is.

I understand the problem is the following (quoted from MSDN
documentation for isspace; other isxxx macros have similar
warnings):

  When used with a debug CRT library, isspace will display a
  CRT assert if passed a parameter that isn't EOF or in the
  range of 0 through 0xFF. When used with a debug CRT library,
  isspace will use the parameter as an index into an array,
  with undefined results if the parameter isn't EOF or in the
  range of 0 through 0xFF.

So the proper range of the parameter is EOF or 0 through 0xFF.

This is why I think negative values (other than EOF) are
illegal.

In my two patches, I never apply the "unsigned char" cast to
EOF.  The functions continue to return EOF without a cast
to "unsigned char".
Comment 17 Wan-Teh Chang 2005-07-15 09:59:54 PDT
Bastiaan: I just found that I misunderstood your remark about
EOF.  Now I understand what you said.

You misunderstood the location of my "unsigned char" cast.
It is inside the FB_GetChar() function.  Your remark would be
true if the "unsigned char" cast were outside the FB_GetChar()
function, like this:

    curchar = (unsigned char) FB_GetChar(fb);

But that's not what my patch does.
Comment 18 Bastiaan Jacques 2005-07-15 10:27:43 PDT
After some more thought, I concluded your fixes are valid after all. In my
comment I made a wrong assumption (but not the one you pointed out) which caused
me to come to the conclusions I did.
Comment 19 Darin Fisher 2005-08-03 12:45:16 PDT
Comment on attachment 189238 [details] [diff] [review]
nspr part, v2 (checked in)

r=darin
Comment 20 Wan-Teh Chang 2005-08-05 15:44:37 PDT
Comment on attachment 189238 [details] [diff] [review]
nspr part, v2 (checked in)

I checked in this patch on the NSPR trunk (NSPR 4.6.1).
Comment 21 Nelson Bolyard (seldom reads bugmail) 2005-08-14 17:50:23 PDT
Comment on attachment 189236 [details] [diff] [review]
nss part, v2 (checked in)

r=nelson@bolyard
Comment 22 Wan-Teh Chang 2005-08-15 15:07:40 PDT
Comment on attachment 189236 [details] [diff] [review]
nss part, v2 (checked in)

I checked in this patch on the NSS trunk (NSS 3.11).
Comment 23 Wan-Teh Chang 2005-08-15 15:09:27 PDT
The NSPR and NSS patches have both been checked in on
their trunks.  They will eventually appear in the Mozilla
clients, hopefully in Mozilla 1.9 Alpha.  I will not seek
mozilla1.8b4 approval because neither of these patches
is critical.

Note You need to log in before you can comment on or make changes to this bug.