[pulseaudio-discuss] [PATCH] netbsd: Fix undefined behavior with array subscript of invalid type

[Arun Raghavan]

On 20 November 2015 at 15:52, Kamil Rytarowski <n54 at gmx.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
>
> On 20.11.2015 07:35, Arun Raghavan wrote:
>> On 20 November 2015 at 08:50, Kamil Rytarowski <n54 at gmx.com>
>> wrote:
>>> From the NetBSD manual:
>>>
>>> The first argument of these functions is of type int, but only a
>>> very restricted subset of values are actually valid.  The
>>> argument must either be the value of the macro EOF (which has a
>>> negative value), or must be a non-negative value within the range
>>> representable as unsigned char. Passing invalid values leads to
>>> undefined behavior.
>>>
>>> --  ctype(3)
>>
>> This is also true for C99 in general, so not a NetBSD-specific
>> thing, it seems.
>>
>
> This is a part of the C Standard. Behind the scenes ctype(3) functions
> are table lookup functions, passing anything out of the range results
> in undefined/invalid memory access.
>
>>> --- src/modules/dbus/iface-core.c |  2 +- src/pulse/proplist.c
>>> | 12 ++++++------ src/pulsecore/core-util.c     |  6 +++---
>>> src/pulsecore/ltdl-helper.c   |  2 +- src/pulsecore/modargs.c
>>> |  8 ++++---- 5 files changed, 15 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/src/modules/dbus/iface-core.c
>>> b/src/modules/dbus/iface-core.c index 1b14195..88e9030 100644 ---
>>> a/src/modules/dbus/iface-core.c +++
>>> b/src/modules/dbus/iface-core.c @@ -1442,7 +1442,7 @@ static bool
>>> contains_space(const char *string) { pa_assert(string);
>>>
>>> for (p = string; *p; ++p) { -        if (isspace(*p)) +        if
>>> (isspace((unsigned char)*p)) return true; }
>>
>> I'm not sure how this is better -- we go from undefined behaviour
>> in the library to forcing potentially undefined behaviour ourselves
>> -- non-ASCII values will generate a "random" value in ASCII space.
>>
>
> The isspace(3) function isn't designed to handle non-ASCII characters.
> If we pass anything (except EOF) we always must cast it, no matter
> what the program logic is.
>
>> We should be checking for valid input instead if we care about
>> this (the one place I quickly checked that uses isspace() is
>> preceded by a pa_asci_valid() call).
>>
>
> This is separated task not handled by this commit.

Okay, that sounds fine. Pushing now.

-- Arun