threads / discuss / 58097

undefined behavior in builtin/am.c

Subject: undefined behavior in builtin/am.c

## tl;dr

4 messages between Jul 1, 2022 and Jul 1, 2022.

replies: 3people: 3as markdown or json

Roland Illig· Jul 1, 2022, 17:03 UTC · lore

Hi,

builtin/am.c says:
 > static int str_isspace(const char *str)
 > {
 > 	for (; *str; str++)
 > 		if (!isspace(*str))
 > 			return 0;
 >
 > 	return 1;
 > }

The macro 'isspace' must only be called with an integer representable as an 'unsigned char', or with the value of the macro EOF.

On platforms where plain 'char' is a signed integer type, any character whose value is negative invokes undefined behavior (except for the one character that by coincidence has the same value as the macro EOF).

To fix this, write '!isspace((unsigned char)*str)' instead.

I have no idea how to trigger this part of the code but for someone who knows this part of Git, it should be easy. Depending on the platform, this kind of error may be silently ignored or crash the program, as always with undefined behavior.

Roland

Jeff King· Jul 1, 2022, 17:53 UTC · re: Roland Illig · lore

Re: undefined behavior in builtin/am.c

On Fri, Jul 01, 2022 at 07:03:18PM +0200, Roland Illig wrote:

Show 13 quoted lines

> The macro 'isspace' must only be called with an integer representable as
> an 'unsigned char', or with the value of the macro EOF.
> 
> On platforms where plain 'char' is a signed integer type, any character
> whose value is negative invokes undefined behavior (except for the one
> character that by coincidence has the same value as the macro EOF).
> 
> To fix this, write '!isspace((unsigned char)*str)' instead.
> 
> I have no idea how to trigger this part of the code but for someone who
> knows this part of Git, it should be easy. Depending on the platform,
> this kind of error may be silently ignored or crash the program, as
> always with undefined behavior.

We don't use the system isspace(), but instead our own macro wrappers in git-compat-util.h. They do the cast to unsigned char themselves.

I won't be surprised if re-defining a system name as a macro is also technically undefined behavior, but I don't think we've found a system that has a problem with it in the past 17 years. :)

-Peff

Roland Illig· Jul 1, 2022, 18:58 UTC · re: Jeff King · lore

Re: undefined behavior in builtin/am.c

Am 01.07.2022 um 19:53 schrieb Jeff King:

Show 18 quoted lines

> On Fri, Jul 01, 2022 at 07:03:18PM +0200, Roland Illig wrote:
>
>> The macro 'isspace' must only be called with an integer representable as
>> an 'unsigned char', or with the value of the macro EOF.
>>
>> On platforms where plain 'char' is a signed integer type, any character
>> whose value is negative invokes undefined behavior (except for the one
>> character that by coincidence has the same value as the macro EOF).
>>
>> To fix this, write '!isspace((unsigned char)*str)' instead.
>>
>> I have no idea how to trigger this part of the code but for someone who
>> knows this part of Git, it should be easy. Depending on the platform,
>> this kind of error may be silently ignored or crash the program, as
>> always with undefined behavior.
>
> We don't use the system isspace(), but instead our own macro wrappers in
> git-compat-util.h. They do the cast to unsigned char themselves.

Thanks for the explanation, good to know that there's no problem. :)

Roland

Phillip Wood· Jul 1, 2022, 17:54 UTC · re: Roland Illig · lore

Re: undefined behavior in builtin/am.c

Hi Roland

On 01/07/2022 18:03, Roland Illig wrote:

Show 14 quoted lines

> Hi,
> 
> builtin/am.c says:
>  > static int str_isspace(const char *str)
>  > {
>  >     for (; *str; str++)
>  >         if (!isspace(*str))
>  >             return 0;
>  >
>  >     return 1;
>  > }
> 
> The macro 'isspace' must only be called with an integer representable as
> an 'unsigned char', or with the value of the macro EOF.

Thanks for reporting this. Git uses its own version of isspace() (see git-compat-util.h & ctype.c) which does not suffer from this limitation as it casts its argument to an unsigned char.

Best Wishes

Phillip

Show 12 quoted lines

> On platforms where plain 'char' is a signed integer type, any character
> whose value is negative invokes undefined behavior (except for the one
> character that by coincidence has the same value as the macro EOF).
> 
> To fix this, write '!isspace((unsigned char)*str)' instead.
> 
> I have no idea how to trigger this part of the code but for someone who
> knows this part of Git, it should be easy. Depending on the platform,
> this kind of error may be silently ignored or crash the program, as
> always with undefined behavior.
> 
> Roland