Vixie-cron bug
Hi,
Vixie-cron-4.1 has been recently added to FC. So far, there have been quite
a few complaints about various aspects of it being broken. Cron is a very
important facility and will upset many people if it doesn't work right. With
the freeze is coming in a few weeks, I want to point out a bug in the program.
I usually work with upstream authors on problems like this and don't call
attention to them publicly. I have contacted the author 10 days ago and sent
him the report. He said he will look at it but it seems open-ended as to when
that will happen. I don't have time to fix this and *verify* the solution at the
moment, but I want to detail this one so others who have time might look into
it.
When I started reviewing the code, I ran across a difference in size
calculations on the dow (day of the week) variable. One place the problem
manifests itself at line 129 of entry.c (there are more places):
bit_set(e->month, 0);
bit_nset(e->dow, 0, (LAST_DOW-FIRST_DOW+1));
e->flags |= DOW_STAR;
For the sake of this discussion, let's take LAST_DOW-FIRST_DOW+1 to be 8.
dow is declared in structs.h as
bitstr_t bit_decl(dow, DOW_COUNT);
bit_decl is:
#define bit_decl(name, nbits) \
(name)[bitstr_size(nbits)]
and
#define bitstr_size(nbits) \
((((nbits) - 1) >> 3) + 1)
So, this means that ((8-1)>>3)+1 is 1. dow is an array 1 byte wide -> dow[0].
The bit_nset macro is where the problem comes in. Inside it is this:
register int _stopbyte = _bit_byte(_stop) <- stop is 8
#define _bit_byte(bit) \
((bit) >> 3)
_bit_byte will come up 1 for stopbyte and 0 for startbyte
This means that it will enter the loop in bit_nset:
_name[_startbyte] |= 0xff << ((_start)&0x7); \
while (++_startbyte < _stopbyte) \
_name[_startbyte] = 0xff; \
_name[_stopbyte] |= 0xff >> (7 - (_stop&0x7)); \
It will do the startbyte assignment, pre-increment startbyte. Now they are equal
and bypasses the while loop. Then it writes to _name[_stopbyte]...aka dow[1].
dow[0] is legal, but dow[1] is out of range. It will inadvertently write into
an adjacent field of the _entry structure.
So, the question boils down to this: is the dow really 2 bytes or 1 byte? A
simple fix is to change bitstr_size (which is only used for dow):
- ((((nbits) - 1) >> 3) + 1)
+ (((nbits) >> 3) + 1)
Now, dow is two bytes. No more out of bounds problems. However, I don't think
this is really the best way to solve the problem. One would think 8 bits could
be held in one byte. I haven't researched the impact of changing _bit_byte
since that macro gets used in more places, but more likely to be the
correct fix.
Anyone want to verify this and/or chase it down?
-Steve Grubb
__________________________________
Do you Yahoo!?
Take Yahoo! Mail with you! Get it on your mobile phone.
http://mobile.yahoo.com/maildemo