[rrd-developers] rrdtool-1.3rc6 is out

[Tobias Oetiker]

Hi Sebastian,

thanks for the patch ... I like the aproach ... will integrate in
1.3 ... in the 1.2 branche I did the flip thing ...

cheers
tobi

Today Sebastian Harl wrote:

> tags 450578 = patch
> thanks
>
> Hi Tobi,
>
> (Please ignore the two lines at the top of this email - they are meant
> for the Debian bug-tracking system.)
>
> Thanks for your reply!
>
> On Fri, May 30, 2008 at 05:55:20PM +0200, Tobias Oetiker wrote:
> > the root cause of the problem is that
> >
> > static char rrd_error[MAXLEN] = "\0";
> > static char rrd_liberror[ERRBUFLEN] = "\0";
> >
> > creates a nice long array but then points it to a rather short string.
>
> I'm sorry, but I fail to see the problem here. I hope, I did not get you
> wrong. That above mentioned code statically allocates two char-arrays of
> sizes MAXLEN and ERRBUFLEN. Initializing the strings with "\0" (btw. ""
> would be equally fine here) does not do any harm - it's just like any
> other initialization in that it sets an initial value for those strings
> but does not affect the amount of memory allocated for the strings.
>
> > I suggest to merge the follwoing patch.
>
> Hrm, as far as I can see it, that patch does not really change anything.
> The real problem is the following:
>
> In src/rrd.h, struct rrd_context is defined as:
>
>   struct rrd_context {
>       int   len;
>       int   errlen;
>       char *lib_errstr;
>       char *rrd_error;
>   }
>
> In src/rrd_not_thread_safe.c an instance of that struct is defined as:
>
>   static struct rrd_context global_ctx = {
>       MAXLEN,
>       ERRBUFLEN,
>       rrd_error,
>       rrd_liberror
>   }
>
> MAXLEN is the size of the char-array called "rrd_error" while ERRBUFLEN
> is the size of the array called "rrd_liberror". So the member "len" is
> used to store the size of the member "lib_errstr". Same applies for the
> members "errlen" and "rrd_error". It's important to note that MAXLEN >
> ERRBUFLEN.
>
> Now, in src/rrd_error.c, the following line appears:
>
>   vsnprintf(CTX->rrd_error, CTX->len, fmt, argp);
>
> What's going on here? We're writing to the string stored in the struct
> member "rrd_error" using the size stored in the member "len" (which
> equals MAXLEN). However, in src/rrd_not_thread_safe.c the size of the
> "rrd_error" member was stored in the member "errlen" (ERRBUFLEN). So,
> we're allowing vsnprintf to write up to MAXLEN = 4096 bytes into a
> string of size ERRBUFLEN = 256, thus possibly causing a segfault.
>
> I hope this did not get too confusing - I, at least, was confused
> multiple times when trying to understand this...
>
> In his patch, Matthew Boyle suggested to switch "rrd_error" and
> "rrd_liberror" in src/rrd_not_thread_safe.c. This would fix the issue as
> now src/rrd_error.c, uses the correct size when accessing the string
> members. Also, this would assign the variable "rrd_error" to the member
> called "rrd_error" which sounds like the original intention of the
> author to me ;-)
>
> Anyway, imho the real source of this problem is the confusion around and
> the error-proneness of having to do the housekeeping of the sizes
> manually. I thus suggest to apply the attached patch which a) solves
> this issue and b) removes that manual housekeeping. Please see the
> description of the patch for a more detailed rationale for it.
> Unfortunately, that patch introduces a non-backward-compatible change
> which would require a SONAME bump (librrd2 -> librrd3). However, I don't
> think that it hurts much to introduce that change in 1.3 - if you're
> planing to do another patch release for 1.2, I'd suggest to apply
> Matthew's patch to the "1.2" branch.
>
> Cheers,
> Sebastian
>
>

-- 
Tobi Oetiker, OETIKER+PARTNER AG, Aarweg 15 CH-4600 Olten
http://it.oetiker.ch tobi at oetiker.ch ++41 62 213 9902