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

[Kamil Rytarowski]

-----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.

> -- Arun _______________________________________________ 
> pulseaudio-discuss mailing list 
> pulseaudio-discuss at lists.freedesktop.org 
> http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJWTvR6AAoJEEuzCOmwLnZsD0cP/01s24I1zTZqtGen3OoX/Sok
Acyp0AlfzahPWeXG/2IaVmCs0g/TyIbATJFbCKSNdaopZQDXtfQ2ZfTr1K3ywl+c
0KBZ5AC91pfZgd+hMxQhzbNEFZ27QuL0fD5fvtqIlaRb3xxkQPDyI2zyAMgD24lk
lamNEJAp8nBvEtuzQ5+gy6NOcZw6D8kMYKnfrIrXNP+TJsuZ0lsrR8lMJcl7Fwi6
F+K9GxJoR1ju3DWbPS2IfCVxOLa3ZYVHtvdG58G+tzQ8G269vblfAs9ZPSo25uGj
Io3hgOPP7F8ll3fDYA2QSLGPTeAdo6AOIEGsKGs2jhe1+HTRHAvhclYihBeOEdxL
EGLPzHsLJOXIS/efr0VrkvsCY/kbAbGrf/hj0TECqsl2jJQ/IpgAVF+2xKxgqBQh
I01lS0T5OvCfhOmvvpPcXQwJmkWwbDVAB8VLSQ/YjQL9G2InEv+NWUXWldXhdyRK
NuY9rO1WI5RTrsnqoCWc4bMHuP6WYohlHXT6J9YZ8AyTQYDnQ4rFoeyZfDLWDQ6F
UtoIKtjA/npaxQ7iVFbJ89XQfX5/6iCa0uwf6ar5DtPUe3ZX/ZPLx90qA5UvXf/m
ghdFalHYnIoyYhEi4gZtWg5j/szP/DarTZrvPQsUedsz7Z6urQ3ae8/o7PjgDBRl
kKeCXDZPbt1tqog4K+6O
=9az3
-----END PGP SIGNATURE-----