[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Vixie-cron bug

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 
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) \
#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.

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]