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 [EMAIL PROTECTED] ++41 62 213 9902
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
