freeradius 3.0.0 fails to start on ppc64 and s390 architectures
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. Attached is a git formatted patch which changes 3 bool declarations to int and include a full explanation in the git commit message. The attachment can be applied with git apply. -- John
On 14 Nov 2013, at 02:48, John Dennis <jdennis@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? Bool is the correct type to use here, the conffile code should be updated to treat booleans as the bool type. It shouldn't be that hard to find where boolean values are using in module configurations, i'll take a look at completing the conversion from int to bool. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On 14 Nov 2013, at 13:29, Arran Cudbard-Bell <a.cudbardb@freeradius.org> wrote:
On 14 Nov 2013, at 02:48, John Dennis <jdennis@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?
Ah no, I guess it won't, because were using offset. Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
On 11/14/2013 08:29 AM, Arran Cudbard-Bell wrote:
On 14 Nov 2013, at 02:48, John Dennis <jdennis@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.
Bool is the correct type to use here, the conffile code should be updated to treat booleans as the bool type.
It shouldn't be that hard to find where boolean values are using in module configurations, i'll take a look at completing the conversion from int to bool.
Arran Cudbard-Bell <a.cudbardb@freeradius.org> FreeRADIUS Development Team
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/devel.html
-- John
John Dennis wrote:
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.
We run Coverity scans daily on the source. All of the issues it currently finds are either false positive, or weird edge conditions. (i.e. didn't free memory just before exit) We've been in touch with the Coverity people since day 1 of their open source scan project. We were part of a presentation they did at Linuxcon in Edinburgh last month. Their comment was that we have a very low defect density. Alan DeKok.
On 14 Nov 2013, at 14:30, John Dennis <jdennis@redhat.com> wrote:
On 11/14/2013 08:29 AM, Arran Cudbard-Bell wrote:
On 14 Nov 2013, at 02:48, John Dennis <jdennis@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@freeradius.org> FreeRADIUS Development Team
participants (3)
-
Alan DeKok -
Arran Cudbard-Bell -
John Dennis