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

[Sebastian Harl]

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
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.

[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.
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.

> 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.

Cheers,
Sebastian

-- 
Sebastian "tokkee" Harl +++ GnuPG-ID: 0x8501C7FC +++ http://tokkee.org/

Those who would give up Essential Liberty to purchase a little Temporary
Safety, deserve neither Liberty nor Safety.         -- Benjamin Franklin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.oetiker.ch/pipermail/rrd-developers/attachments/20080611/ba76c79b/attachment.bin