[rrd-developers] rrdtool: *** glibc detected *** free(): invalid pointer: 0x08056450 ***

[Bernhard Fischer]

On Wed, Jun 11, 2008 at 10:28:47PM +0200, Sebastian Harl wrote:
>Hi,
>
>I'm keeping this to rrd-developers now, since that should no longer be
>really interesting for the Debian bug report.
>
>On Wed, Jun 11, 2008 at 07:52:01PM +0200, Bernhard Fischer wrote:
>> On Wed, Jun 11, 2008 at 07:03:48PM +0200, Sebastian Harl wrote:
>> >On Wed, Jun 11, 2008 at 10:50:17AM +0200, Sebastian Harl wrote:
>> >> On Sun, Nov 18, 2007 at 10:49:21PM +0100, Matej Kosik wrote:
>> >> > when I perform the following command
>> >> > 
>> >> > rrdtool graph /tmp/plot_1195420761938267.png --width 479 --height 71 --start 1195257600 --end 1195344000 -i -y 1:1 -l 0 -u 1 DEF:mystate=node_0.rrd:state:AVERAGE LINE2:mystate#FF0000
>> >> > 
>> >> > I get an error:
>> >> > 
>> >> > *** glibc detected *** free(): invalid pointer: 0x08056450 ***
>> >> > 
>> >> > and no graph is generated. The node_0.rrd file was created as follows:
>> >> > 
>> >> > 	rrdtool create /usr/local/src/nms/nms/priv/rrd/node_0.rrd -s 60000 DS:state:GAUGE:60:U:U RRA:AVERAGE:0.5:1:604800
>> >[...]
>> >>   start_offset = (long) (*start + *step - rra_start_time) / (long) *step;
>> >> 
>> >> I suspect that we're getting some kind of overflow here. Tobi do you have any
>> >> ideas about that?
>> >
>> >Okay, so here is the real issue:
>> >
>> >In this case a fairly large row count (604800) and a really unusually
>> >large step size (60000) was used. Now, the critical part of
>> >rrd_fetch_fn() is the calculation of rra_start_time (which in turn is
>> >then used to calculate start_offset):
>> >
>> >  rra_start_time = (rra_end_time
>> >                   - (*step * (rrd.rra_def[chosen_rra].row_cnt - 1)));
>> >
>> >step * (row_cnt - 1) equals 36287940000 which is way bigger than
>> >LONG_MAX on 32bit architectures (which usually equals 2147483647). So,
>> >we end up with an overflow of the "long int" value and rra_start_time
>> 
>> Didn't look, admittedly, but aren't they unsigned long int and could
>> eventually be unsigned long long int (which would potentially hurt
>> alot)?
>
>Yes, switching from "long" to "long long" might be one way to implement
>that. Iirc, "long long" has been added in C99 and most current compilers
>should support it, so I don't really think we will run into much trouble

Don't drop that unsigned. It makes a bit of difference ;)

>because of that. collectd [1] is using "long long"'s in a couple of
>places and it's known to compile and run on quite a few different
>architectures.

I'm not talking about whether it runs or not, i'm talking about how much
penalty you pay for using it on arches that don't do it natively.
Folks are adding 256bit support for certain areas and codes, still you
don't want to *emulate* 256bit support or anything else on a let's say
32bit host or even 64bit hosts.
>
>[1] http://collectd.org/
>
>> I think that we should better avoid the trouble right on spot and try to
>> calculate the upcoming wrapping beforehand.
>
>Well, those values are later used as offsets into the RRD file.

exactly, and will wrap, rra-wise, see below.

>Admittedly, it should be quite uncommon to have RRD files exceeding 2GB
>but if adding support for those cases is doable in a fairly reasonable
>way, it should be done anyway.
>
>> >containing some nonsense value.
>> >
>> >I'm currently unsure how to solve that properly. It might make sense to
>> >switch to some 64bit type here - that's what's required by the LFS
>> 
>> LFS itself optional, so no, not required.
>
>Well, yes - I meant to say that this is required to implement LFS, which
>Imho would be a nice thing to have.

I don't want nor need it, but supporting it should be really trivial.

>> We can test if it will wrap and should be (possibly) able to
>> precalculate it here instead of later on.
>
>I'm not sure what you wanted to say with that... If it wraps and we
>don't use any data types that are able to cope with that, I don't think
>there's anything else we can do but bail out with an error message.

I'm not talking about an overflow but wrap, really.
$ grep wrap src/*fetch*
If it doesn't overflow, it would wrap, so prevent it from overflowing in
the first place, that's what i ment. See?