Audit consumers of isspace

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Internationalization
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: timeless, Assigned: Wan-Teh Chang)

Tracking

Trunk
mozilla1.9alpha1
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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

12 years ago
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

12 years ago
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.
I agree with comment 1, assuming that by "all our usage" you are only referring
to usage with PRUnichar or equivalent.

Comment 4

12 years ago
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

12 years ago
..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().

Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 6

12 years ago
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

12 years ago
Created attachment 188730 [details] [diff] [review]
nss part

Comment 8

12 years ago
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

12 years ago
Created attachment 189088 [details] [diff] [review]
nss part (compiles)
Attachment #188730 - Attachment is obsolete: true
Attachment #189088 - Flags: review?(wtchang)

Comment 10

12 years ago
Created attachment 189089 [details] [diff] [review]
nspr part (compiles)
Attachment #188731 - Attachment is obsolete: true
Attachment #189089 - Flags: review?(cls)

Updated

12 years ago
Attachment #189089 - Flags: review?(cls) → review?(wtchang)
(Assignee)

Comment 11

12 years ago
Related macros like isdigit and isalpha have the
same problem, right?
(Assignee)

Comment 12

12 years ago
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().
Attachment #189088 - Flags: review?(wtchang) → review-
(Assignee)

Comment 13

12 years ago
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."
Attachment #189088 - Attachment is obsolete: true
(Assignee)

Comment 14

12 years ago
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)
(Assignee)

Updated

12 years ago
Attachment #189089 - Attachment is obsolete: true

Updated

12 years ago
Attachment #189089 - Flags: review?(wtchang)

Comment 15

12 years ago
(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;
(Assignee)

Comment 16

12 years ago
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".
(Assignee)

Comment 17

12 years ago
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

12 years ago
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.

Updated

12 years ago
Assignee: b.jacques → wtchang
(Assignee)

Updated

12 years ago
Attachment #189238 - Flags: review?(darin)
(Assignee)

Updated

12 years ago
Attachment #189236 - Flags: review?(nelson)

Comment 19

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

r=darin
Attachment #189238 - Flags: review?(darin) → review+
(Assignee)

Comment 20

12 years ago
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).
Attachment #189238 - Attachment description: nspr part, v2 → nspr part, v2 (checked in)
Comment on attachment 189236 [details] [diff] [review]
nss part, v2 (checked in)

r=nelson@bolyard
Attachment #189236 - Flags: review?(nelson) → review+
(Assignee)

Comment 22

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

I checked in this patch on the NSS trunk (NSS 3.11).
Attachment #189236 - Attachment description: nss part, v2 → nss part, v2 (checked in)
(Assignee)

Comment 23

12 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
You need to log in before you can comment on or make changes to this bug.