298612 - Audit consumers of isspace

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[timeless]

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]

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]

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]

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]

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]

..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]

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]

Attachment: nss part

Comment 8

[Bastiaan Jacques]

Attachment: nspr part

I assume the sqlite changes will have to be done by the sqlite project itself.

Comment 9

[Bastiaan Jacques]

Attachment: nss part (compiles)

Comment 10

[Bastiaan Jacques]

Attachment: nspr part (compiles)

Comment 11

[Wan-Teh Chang]

Related macros like isdigit and isalpha have the
same problem, right?

Comment 12

[Wan-Teh Chang]

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]

Attachment: 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]

Attachment: 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]

(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]

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]

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]

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]

Comment on attachment 189238 [details] [diff] [review]
nspr part, v2 (checked in)

r=darin

Comment 20

[Wan-Teh Chang]

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)]

Comment on attachment 189236 [details] [diff] [review]
nss part, v2 (checked in)

r=nelson@bolyard

Comment 22

[Wan-Teh Chang]

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]

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.