Hi all,

While experimenting with NetBSD/prep code, I noticed a coding style
flaw. It is probably very unlikely to trigger bugs, but I think it is
worth mentioning it anyway.

A quick googling turned up that this flaw was noticed by Simon Burge in
May 2003 (http://mail-index.netbsd.org/tech-kern/2003/05/07/0004.html).
Unfortunately, there is still code around which is not rewritten.

Basically, the FROMBCD macro is misused if it is given a function call
as an argument, instead of a plain variable.

     #define  FROMBCD(x)      (((x) >> 4) * 10 + ((x) & 0xf))

In sys/arch/prep/isa/mcclock_isa.c, the value from mcclock_isa_read()
can differ between two consecutive reads.

     t1 = FROMBCD(mcclock_isa_read(sc, MC_SEC));

could return 40 or 59, if the RTC's MC_SEC value changes from 0x49 to
0x50 between the two reads. (The value can be either 40 or 59, because
it is not well-defined which of the two (x)s that gets evaluated first.)

In most cases, this would only lead to a scenario where the machine
thinks that the time is a few seconds more/less than what the RTC
actually thinks. I think this is what Simon noticed.

For the PReP port, it is slightly more serious. mcclock and mkclock
check for the existence of a clock chip by 1) reading MC_SEC, 2) doing a
hardcoded delay of 1.1 seconds, 3) reading MC_SEC again. If the
difference between the first and second MC_SEC is exactly 1 or 2, then
the clock chip is assumed to be working.

If neither mcclock or mkclock is found working, then there will be a
NULL pointer dereference (and a crash) later on during boot in
inittodr().

The solution is either to do what Simon suggested (changing the macro
into a function), or to simply read the value into a temporary variable
before using the macro. (More places than just the prep code might be
affected.)

Again, this is probably very unlikely to lead to crashes (especially on
real hardware), but this way of using macros when accessing hardware
registers should be avoided.


Anders