freeradius 3.0.0 fails to start on ppc64 and s390 architectures

[Arran Cudbard-Bell]

On 14 Nov 2013, at 14:30, John Dennis <jdennis at redhat.com> wrote:

> On 11/14/2013 08:29 AM, Arran Cudbard-Bell wrote:
>> 
>> On 14 Nov 2013, at 02:48, John Dennis <jdennis at redhat.com> wrote:
>> 
>>> On the ppc64 and s390 architectures radiusd fails to start emitting
>>> these errors:
>>> 
>>> Error: /etc/raddb/mods-enabled/echo[34]: Cannot read output pairs if
>>> wait = no
>>> Error: /etc/raddb/mods-enabled/echo[34]: Instantiation failed for module
>>> "echo"
>>> 
>>> The problem is due to using the wrong type in the module config struct
>>> passed to the config file parser. The type was declared as bool but
>>> conffile.c treats PW_TYPE_BOOLEAN as an int. This causes an if test to
>>> fail because the bool value is not true when it should be. This is in
>>> rlm_exec.c. I found 1 other module which had bool's declared for
>>> PW_TYPE_BOOLEAN config items, rlm_pap.c.
>> 
>> Out of interest did the compiler emit warnings about the size mismatch? 
> 
> No compiler warnings. I also did a Coverity scan of the source tree and
> although a number of warnings were reported this issue was not detected.
> However I'm not surprised, the way the code works with pointer offsets
> to void * (untyped) targets it would be remarkable if a tool could
> deduce incorrect type declarations.

Quite.

Ok i've pushed a proper fix for this.

Converting those fields to int would of fixed the immediate problem,
but other issues would of arisen later, because of the size mismatch.

Some modules already used the bool type in their instance data structs,
and the config parser would of overwritten the subsequent 3 bytes of
the struct with zeros.

Using ints to represent boolean values makes the code harder to follow,
especially when they are used as function return types, as it's unclear
whether the function will use the standard 0, -1 -n return codes or 
1, 0 return codes from reading the signature.

Once you start using them in one place, not using them in others is 
confusing, and would eventually end up making the code more complex.

Arran Cudbard-Bell <a.cudbardb at freeradius.org>
FreeRADIUS Development Team