Subject: One odd Purify bug found

From: [email-masked] (Paul D. Smith)
Date: 01 Dec 1998 12:19:36 -0500

OK, I found out why we were seeing those extremely odd ABR (Array Bounds
Read) errors in DoNextToken, where the memory was some global value.

As one example, here's a statement where we were getting ABRs;
Parse.c:DoGetNextToken(), line 170:

         while ( (*t != 0) &&
                 ( isspace(*t) ||
                   (snum &&
                    strchr(spaces, *t)) ) )

The ABR actually happens here on the isspace() call.  The problem is
that in the purify .fvwm2rc file Dominik tests with he inserted a bunch
of non-ASCII chars:

  GlobalOpts äöwwwkjfd 2wäeöqdpef

etc.  Now, my locale is, of course, set to the basic C locale.  And the
man page for isspace() says:

     These macros classify  character-coded  integer  values.   Each  is  a
     predicate  returning  non-zero for true, zero for false.  The behavior
     of these macros, except isascii(), is affected by the  current  locale
     (see  setlocale(3C)).   To  modify  the  behavior,  change the LC_TYPE
     category in setlocale(), that is, setlocale(LC_CTYPE, newlocale).   In
     the  C  locale, or in a locale where character type information is not
     defined, characters are classified according to the rules of  the  US-
     ASCII 7-bit coded character set.

So, this function (isspace()) is not defined for non-ASCII, non-7-bit
characters unless you've executed the proper setlocale() or whatever to
make them legal.

In my locale, this function is probably simply doing a table lookup and
since the umlaut "a" above is -28 as a signed char, it's accessing
_backwards_ before the beginning of the array.

So.

We appear to have these choices:

 a) Remove the non-ASCII chars from the Purify test.  This doesn't
    really solve the problem if people do what I was doing (have
    non-ASCII chars in their .fvwm2rc files without the right locale
    set).

 b) Require that people running the Purify test use some locale that is
    defined for these chars (what would that be?  I'm not thrilled by
    this option :).  Again, this doesn't solve the problem in general.

 c) Change all the code that uses these functions to something like:

        isascii(*t) && isspace(*t)

    since isascii() is defined for all chars.  I tested this and it does
    solve the problem.  The issue here, of course, is that it restricts
    the fvwm2rc file to ASCII (7-bit) chars--at least for parsing spaces
    (are there any languages where whitespace chars appear in the upper
    ranges?).

Other options?

-- 
-------------------------------------------------------------------------------
 Paul D. Smith <[email-masked]>         Network Management Development
 "Please remain calm...I may be mad, but I am a professional." --Mad Scientist
-------------------------------------------------------------------------------
   These are my opinions---Nortel Networks takes no responsibility for them.
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: Dan Espen <[email-masked]>
Date: Tue, 01 Dec 1998 12:40:58 -0500

> We appear to have these choices:

If I understand what you are describing, you say your "isspace" function
is taking the character in question and using it as an index into an
array?

I assume this array is 256 bytes long?

If it is, and the characters are signed, shouldn't it adjust the offset
into the table?  (Start in the middle?)

Would making the characters unsigned help?

> -- 
>  Paul D. Smith <[email-masked]>         Network Management Developmen

--
Dan Espen                                Phone: (732) 699-5570
444 Hoes Lane  Room RRC 1C-214           E-mail: [email-masked]
Piscataway, NJ 08854
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: Todd Larason <[email-masked]>
Date: Tue, 1 Dec 1998 10:08:18 -0800

> _backwards_ before the beginning of the array.

The arguments to the is*() functions are ints, and must have a value that 
corresponds to either an unsigned char or EOF.  -28 isn't a valid argument
unless EOF is -28, which is unlikely.
-- 
ICQ UIN: 76068732
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: [email-masked] (Paul D. Smith)
Date: 01 Dec 1998 13:08:49 -0500

%% Dan Espen <[email-masked]> writes:

  de> If I understand what you are describing, you say your "isspace"
  de> function is taking the character in question and using it as an
  de> index into an array?

  de> I assume this array is 256 bytes long?

  de> If it is, and the characters are signed, shouldn't it adjust the
  de> offset into the table?  (Start in the middle?)

Well, this all kind of misses the point I think.  I don't know, for
sure, how the isspace() function/macro on my system (Solaris 2.5.1;
originally found on SunOS 4.1.4, but I'm compiling with GCC so it may be
an inline function or something) is implemented... I'm just making a
guess based on what Purify says.

The point is that the man page says isspace() is _only_ defined across
the valid character set for the locale you're running the program in.
In the absence of settings to the contrary, the default locale is "C",
which is the basic ASCII character set, so isspace() has undefined
behavior on any character which is not a valid ASCII (7-bit) character.

We certainly can't go changing the operating system implementation of
isspace() :)

  de> Would making the characters unsigned help?

Possibly, but if so only by an accident of implementation.  The
important thing to note here is that the behavior is undefined, so we
shouldn't be doing it.

The question is, do we let people use non-ASCII chars in fvwm2rc files
and just assume that if they do so, they'll set the locale correctly?
In this case I think we should remove the 8-bit chars from the purify
tests.

Or do we hardcode restrictions (like using isascii()) and either hope it
works (whitespace appears in the 7-bit char sets for all locales) or
acknowledge that it doesn't?

---

From: Dan Espen <[email-masked]>
Date: Tue, 01 Dec 1998 13:34:29 -0500

> unless EOF is -28, which is unlikely.

I still agree with Todd.  I looked at the implementation of isspace
in ctype.h and it looks to me like using unsigned chars will kill
the ABR.

---

From: [email-masked] (Paul D. Smith)
Date: 01 Dec 1998 14:00:05 -0500

de> I still agree with Todd.  I looked at the implementation of isspace
  de> in ctype.h and it looks to me like using unsigned chars will kill
  de> the ABR.

OK.

I assume we're not going to go through the code and change every
occurance of a char* relating to reading or handling config files to be
a unsigned char*?

So, then, I guess the idea is we change:

  isspace(*p)

to something like:

  isspace((unsigned char)*p)

This does appear to resolve the ABR in my tests.

---

From: Dan Espen <[email-masked]>
Date: Tue, 01 Dec 1998 14:50:16 -0500

[email-masked] (Paul D. Smith) writes:
>   de> (Try this)...

Not too long ago, we discussed patches to fvwm2 for >8bit character sets.
Part of the patch changed "isspace" like this:

isspace(*p & 0xFF)

Does the cast, above, fix the ABR and the >8bit issue?

Macros come to mind, but I don't want to bring up macros.

I  sort of like to know  the sign of characters,  so  I try to declare
chars as unsigned chars...

---

From: [email-masked] (Paul D. Smith)
Date: 02 Dec 1998 21:07:04 -0800

de> Not too long ago, we discussed patches to fvwm2 for >8bit character sets.
  de> Part of the patch changed "isspace" like this:

  de> isspace(*p & 0xFF)

I don't quite get this.  p is of type char*; isn't a char always 8 bits?
Don't you have to use wchar_t or whatever to get wide (>8 bit)
characters?

If I'm right, then isn't the "&0xFF" superfluous?

---

From: Dan Espen <[email-masked]>
Date: Thu, 03 Dec 1998 09:05:22 -0500

[email-masked] (Paul D. Smith) writes:
>   de> Not too long ago, we discussed patches to fvwm2 for >8bit character
>   de> sets. Part of the patch changed "isspace" like this:
> 
>   de> isspace(*p & 0xFF)
> 

I'm   not sure, perhaps   Makoto  MATSUSHITA  hasn't   given up on  us
completely and can explain this.

I would guess that somehow, the >8bit characters have to get into fvwm
as  window titles, etc.  and  they come in   as  ints with very  large
values.

Searches in the mail archive for I18N bring back most of what I remember
on this.  Looking thru the patch again, almost all of the "isspace"
changes are for casts to "unsigned char".  A few of them, go a little
farther:

- while(((isspace(*tmp))
+ while(((isspace((unsigned char)*tmp & 0xff))

The bulk  of the rest of  the  patch seems  to have  to do  with using
"fontsets" instead  of "fonts".  I'm  afraid I'm  a little ignorant in
this area, so I don't really understand why that part was needed.