[Bug libfortran/95177] New: error: array subscript has type char

[Bug libfortran/95177] New: error: array subscript has type char

[roland.illig at gmx dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

            Bug ID: 95177
           Summary: error: array subscript has type char
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libfortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: roland.illig at gmx dot de
  Target Milestone: ---

Created attachment 48552
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48552&action=edit
patch

The GCC code should correctly use the <ctype.h> functions.

Compiling GCC with -Werror=char-subscripts should succeed.

[Bug libfortran/95177] error: array subscript has type char

[kargl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kargl at gcc dot gnu.org

--- Comment #1 from kargl at gcc dot gnu.org ---
Why cast to unsigned char?  The prototypes for tolower(), toupper(),
isdigit(), etc show that the type of the argument is int.

Also, why are errors being issued?  None of the places where 
you have inserted an (unsigned char) cast is a subscripts.

[Bug libfortran/95177] error: array subscript has type char

[roland.illig at gmx dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #2 from Roland Illig <roland.illig at gmx dot de> ---
>--- Comment #1 from kargl at gcc dot gnu.org ---
>Why cast to unsigned char?  The prototypes for tolower(), toupper(),
>isdigit(), etc show that the type of the argument is int.

See https://stackoverflow.com/a/60696378 for a detailed explanation.

[Bug libfortran/95177] error: array subscript has type char

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |egallager at gcc dot gnu.org
           Keywords|                            |build, diagnostic
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=94182

--- Comment #3 from Eric Gallager <egallager at gcc dot gnu.org> ---
(In reply to Roland Illig from comment #2)
> >--- Comment #1 from kargl at gcc dot gnu.org ---
> >Why cast to unsigned char?  The prototypes for tolower(), toupper(),
> >isdigit(), etc show that the type of the argument is int.
> 
> See https://stackoverflow.com/a/60696378 for a detailed explanation.

...which in turn led to bug 94182

[Bug libfortran/95177] error: array subscript has type char

[sgk at troutmask dot apl.washington.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #4 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Tue, May 19, 2020 at 04:38:32AM +0000, roland.illig at gmx dot de wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177
> 
> --- Comment #2 from Roland Illig <roland.illig at gmx dot de> ---
> >--- Comment #1 from kargl at gcc dot gnu.org ---
> >Why cast to unsigned char?  The prototypes for tolower(), toupper(),
> >isdigit(), etc show that the type of the argument is int.
> 
> See https://stackoverflow.com/a/60696378 for a detailed explanation.
> 

Ah, yeah, so?

There are no subscripts in the code you are changing.
Why does -Werror=char-subscripts trigger if there are
no subscripts?  Is the error flag misnamed?

If you're going to fix unbroken code, why not cast the
argument to the declared type of the ctype functions?

[Bug libfortran/95177] error: array subscript has type char

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |msebor at gcc dot gnu.org

--- Comment #5 from Martin Sebor <msebor at gcc dot gnu.org> ---
Calling toupper() or any other character classification function declared in
<ctype.h> with a negative value other than EOF is undefined.  When char is a
signed type, using any value outside the 7-bit ASCII set runs the risk of
accessing the char classification array, commonly used to implement the
functions, outside its bounds due to sign extension.  The Stack Overflow post
describes the technique in the abstract.  An example of a real implementation
is Glibc (see for instance its __isctype macro in <ctype.h>).

Glibc uses casts or other conversions from char to a signed type before using
the character value which suppresses GCC's -Wchar-subscripts, but the problem
still exists.

To avoid the out-of-bounds access the argument to these functions should be
cast to unsigned char first.  This is described in some detail in the CERT C
Secure Coding Standard rule STR37-C. Arguments to character-handling functions
must be representable as an unsigned char:
https://wiki.sei.cmu.edu/confluence/x/BNcxBQ.

[Bug libfortran/95177] error: array subscript has type char

[msebor at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Martin Sebor <msebor at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=78155

--- Comment #6 from Martin Sebor <msebor at gcc dot gnu.org> ---
See also pr78155 for a request to get GCC to warn for some instances of this
problem.

[Bug libfortran/95177] error: array subscript has type char

[tkoenig at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Thomas Koenig <tkoenig at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tkoenig at gcc dot gnu.org

--- Comment #7 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
As a matter of style, I do not like random casts, they clutter the code and
make
adhering to the 80 character line limit even more of a pain.

A macro might be better here, or a function safe_toupper.

[Bug libfortran/95177] error: array subscript has type char

[tkoenig at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Thomas Koenig <tkoenig at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2020-05-20

--- Comment #8 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
Alternatively, it might make sense to change some variable types to
GFC_UINTEGER_1 aka unsigned char.

[Bug libfortran/95177] error: array subscript has type char

[sgk at troutmask dot apl.washington.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #9 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Wed, May 20, 2020 at 04:10:50AM +0000, tkoenig at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177
> 
> Thomas Koenig <tkoenig at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>      Ever confirmed|0                           |1
>              Status|UNCONFIRMED                 |NEW
>    Last reconfirmed|                            |2020-05-20
> 
> --- Comment #8 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
> Alternatively, it might make sense to change some variable types to
> GFC_UINTEGER_1 aka unsigned char.
> 

That could work.  I'm still trying to understand how an
option names -Werror=char-subscripts could trigger an
error.  There are no subscripts.  AFAIK, the patched 
routines are not general purpose routines, so the char
arguments can only take on values from the Fortran
character set, which is a subset of the 7-bit ASCII
set.  

Just commit the patch.  I've wasted too much time trying
to get an answer about how the option works.  Nothing
like cluttering working code.

[Bug libfortran/95177] error: array subscript has type char

[roland.illig at gmx dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #10 from Roland Illig <roland.illig at gmx dot de> ---
(In reply to Steve Kargl from comment #9)
> That could work.  I'm still trying to understand how an
> option names -Werror=char-subscripts could trigger an
> error.  There are no subscripts.

The C standard allows every library function to also be defined as a macro.
This also applies to the functions from <ctype.h>.

The typical implementation of the <ctype.h> functions is:

extern int __ctype_classes[1 /* for EOF */ + UCHAR_MAX];

int isalpha(int ch) {
  return (__ctype_classes + 1)[ch] & 0x0008;
}

The corresponding macro is defined like this:

#define isalpha(ch) ((__ctype_classes + 1)[(ch)] & 0x0008)

That's where the subscript comes from.

The macro has no implicit type conversion (at least on NetBSD; the GNU libc may
differ) and thus produces the warning.

[Bug libfortran/95177] error: array subscript has type char

[egallager at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Eric Gallager <egallager at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=17898

--- Comment #11 from Eric Gallager <egallager at gcc dot gnu.org> ---
related to bug 17898

[Bug libfortran/95177] error: array subscript has type char

[anlauf at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

anlauf at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |anlauf at gcc dot gnu.org

--- Comment #12 from anlauf at gcc dot gnu.org ---
Any reason not to use the macros from safe-ctype.h?

[Bug libfortran/95177] error: array subscript has type char

[tkoenig at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #13 from Thomas Koenig <tkoenig at gcc dot gnu.org> ---
(In reply to anlauf from comment #12)

> Any reason not to use the macros from safe-ctype.h?

Can we actually use it?  This is part of libiberty, AFAIK,
and I am not sure that we link libgfortran against it.

[Bug libfortran/95177] error: array subscript has type char

[anlauf at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #14 from anlauf at gcc dot gnu.org ---
(In reply to Thomas Koenig from comment #13)
> (In reply to anlauf from comment #12)
> 
> > Any reason not to use the macros from safe-ctype.h?
> 
> Can we actually use it?  This is part of libiberty, AFAIK,
> and I am not sure that we link libgfortran against it.

Right.  I didn't actually check.

So instead of using safe-ctype.h's

#define TOUPPER(c) _sch_toupper[(c) & 0xff]

we could do

#define TOUPPER(c) toupper ((c) & 0xff)

which might solve the signedness issue as well as out-of-bounds accesses.
Keeps line length unchanged.  Unless I am missing something else again...

[Bug libfortran/95177] error: array subscript has type char

[roland.illig at gmx dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #15 from Roland Illig <roland.illig at gmx dot de> ---
(In reply to Steve Kargl from comment #9)
> I'm still trying to understand how an
> option names -Werror=char-subscripts could trigger an
> error.  There are no subscripts.

There _are_ subscripts, just look at the precompiled C translation unit.

$ printf '%s\n' '#include <ctype.h>' 'isspace(ch)' | gcc -E -

On Cygwin, the output ends with:

    ((((__locale_ctype_ptr ()) + sizeof(""[ch]))[(int)(ch)])&010)

The expression `sizeof(""[ch])` has two purposes. The first is to add 1 to the
array index (since EOF is -1 in that implementation), the second is to trigger
the GCC diagnostic, to prevent the undefined behavior. That's why all C
programmers who know the <ctype.h> functions cast the argument from `char` to
`unsigned char`, to be absolutely sure that there is no undefined behavior, no
matter what value the argument has.

On NetBSD, the pattern is similar:

    ((int)((_ctype_tab_ + 1)[(ch)] & 0x0040))

Here, the argument is directly used as an array subscript, without casting it
first, with the same effect of triggering the GCC diagnostic.

[Bug libfortran/95177] error: array subscript has type char

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fxcoudert at gcc dot gnu.org

--- Comment #16 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
I feel like the "array subscript has type char" error is really a red herring
in this case. What really matters is the following: throughout libgfortran, we
are calling functions from <ctype.h> that should only be called on values that
can be represented by unsigned char, with char arguments. On targets where char
is a signed type, this will could lead to undefined behaviour for some values.
Because the functions are sometimes called on user-provided strings (strings,
string formats, etc) we cannot guarantee that the strings will not contain
ASCII only. So we have to solve this issue.

The four functions currently used in libgfortran are: isalnum, isdigit,
tolower, toupper. Patch incoming.

[Bug libfortran/95177] error: array subscript has type char

[roland.illig at gmx dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #17 from Roland Illig <roland.illig at gmx dot de> ---
(In reply to Francois-Xavier Coudert from comment #16)
> I feel like the "array subscript has type char" error is really a red
> herring in this case.

I agree, especially for C programmers who are not aware that the functions from
<ctype.h> are most often implemented as macros. GCC should try to bridge this
gap and provide a more accurate, more helpful diagnostic in cases like these.

> The four functions currently used in libgfortran are: isalnum, isdigit,
> tolower, toupper. Patch incoming.

I already attached a patch 18 months ago, using the standard pattern that
everyone else uses to properly fix the type mismatch. I don't think much has
changed in the meantime, so my patch should still apply.

Here are some ideas for implementing this check in a more direct way:

https://github.com/NetBSD/src/blob/trunk/usr.bin/xlint/lint1/ckctype.c
https://github.com/NetBSD/src/blob/trunk/tests/usr.bin/xlint/lint1/msg_341.c
https://github.com/NetBSD/src/blob/trunk/tests/usr.bin/xlint/lint1/msg_342.c

[Bug libfortran/95177] error: array subscript has type char

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

--- Comment #18 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Patch submitted at
https://gcc.gnu.org/pipermail/fortran/2021-December/057198.html

[Bug libfortran/95177] error: array subscript has type char

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #19 from Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> ---
Patch committed, bug fixed in mainline.

[Bug libfortran/95177] error: array subscript has type char

[fxcoudert at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95177

Francois-Xavier Coudert <fxcoudert at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0