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
--
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
From 517f04948657e30cd529ea16887aa2d1e97f42d9 Mon Sep 17 00:00:00 2001
From: Sebastian Harl <[EMAIL PROTECTED]>
Date: Sun, 1 Jun 2008 01:10:03 +0200
Subject: [PATCH] Simplified struct rrd_context and its handling.
The struct rrd_context contains two strings to store error messages. Up to
now, a char-pointer was used for those members and two additional members
where provided to store the size of each string. Unfortunately, this led
to bugs causing segfaults because the mapping of size and string members
got mixed up.
This patch replaces the char-pointers with char-arrays of fixed size. This
adds the benefit that sizeof() may be used to automatically determine the
size of each string removing the need for any housekeeping of that kind of
information and, thus, should remove any source of confusion. So, the size
members are no longer required and have been removed. Also, any instance
of that struct already used the same fixed size for the strings, so this
should not introduce any drawbacks.
This should fix Debian bug #450578.
Signed-off-by: Sebastian Harl <[EMAIL PROTECTED]>
---
program/src/rrd.h | 6 ++----
program/src/rrd_error.c | 36 +++++++++++-------------------------
program/src/rrd_not_thread_safe.c | 15 ++-------------
program/src/rrd_thread_safe.c | 6 +++---
program/src/rrd_thread_safe_nt.c | 4 ++--
5 files changed, 20 insertions(+), 47 deletions(-)
diff --git a/program/src/rrd.h b/program/src/rrd.h
index 3007026..93520c0 100644
--- a/program/src/rrd.h
+++ b/program/src/rrd.h
@@ -231,10 +231,8 @@ extern "C" {
/* END parsetime.h */
struct rrd_context {
- int len;
- int errlen;
- char *lib_errstr;
- char *rrd_error;
+ char lib_errstr[256];
+ char rrd_error[4096];
};
/* returns the current per-thread rrd_context */
diff --git a/program/src/rrd_error.c b/program/src/rrd_error.c
index 1d7ae6b..4eb9d59 100644
--- a/program/src/rrd_error.c
+++ b/program/src/rrd_error.c
@@ -46,7 +46,7 @@ void rrd_set_error(
rrd_clear_error();
va_start(argp, fmt);
#ifdef HAVE_VSNPRINTF
- vsnprintf(CTX->rrd_error, CTX->len, fmt, argp);
+ vsnprintf(CTX->rrd_error, sizeof(CTX->rrd_error), fmt, argp);
#else
vsprintf(CTX->rrd_error, fmt, argp);
#endif
@@ -87,10 +87,10 @@ void rrd_set_error_r(
rrd_clear_error_r(rrd_ctx);
va_start(argp, fmt);
#ifdef HAVE_VSNPRINTF
- vsnprintf((char *) rrd_ctx->rrd_error, rrd_ctx->len, fmt, argp);
- rrd_ctx->rrd_error[rrd_ctx->len] = '\0';
+ vsnprintf(rrd_ctx->rrd_error, sizeof(rrd_ctx->rrd_error), fmt, argp);
+ rrd_ctx->rrd_error[sizeof(rrd_ctx->rrd_error) - 1] = '\0';
#else
- vsprintf((char *) rrd_ctx->rrd_error, fmt, argp);
+ vsprintf(rrd_ctx->rrd_error, fmt, argp);
#endif
va_end(argp);
}
@@ -110,7 +110,7 @@ void rrd_clear_error_r(
char *rrd_get_error_r(
struct rrd_context *rrd_ctx)
{
- return (char *) rrd_ctx->rrd_error;
+ return rrd_ctx->rrd_error;
}
#endif
@@ -122,33 +122,19 @@ struct rrd_context *rrd_new_context(
struct rrd_context *rrd_ctx =
(struct rrd_context *) malloc(sizeof(struct rrd_context));
- if (rrd_ctx) {
- rrd_ctx->rrd_error = malloc(MAXLEN + 10);
- rrd_ctx->lib_errstr = malloc(ERRBUFLEN + 10);
- if (rrd_ctx->rrd_error && rrd_ctx->lib_errstr) {
- *rrd_ctx->rrd_error = 0;
- *rrd_ctx->lib_errstr = 0;
- rrd_ctx->len = MAXLEN;
- rrd_ctx->errlen = ERRBUFLEN;
- return rrd_ctx;
- }
- if (rrd_ctx->rrd_error)
- free(rrd_ctx->rrd_error);
- if (rrd_ctx->lib_errstr)
- free(rrd_ctx->lib_errstr);
- free(rrd_ctx);
+ if (! rrd_ctx) {
+ return NULL;
}
- return NULL;
+
+ rrd_ctx->rrd_error[0] = '\0';
+ rrd_ctx->lib_errstr[0] = '\0';
+ return rrd_ctx;
}
void rrd_free_context(
struct rrd_context *rrd_ctx)
{
if (rrd_ctx) {
- if (rrd_ctx->rrd_error)
- free(rrd_ctx->rrd_error);
- if (rrd_ctx->lib_errstr)
- free(rrd_ctx->lib_errstr);
free(rrd_ctx);
}
}
diff --git a/program/src/rrd_not_thread_safe.c
b/program/src/rrd_not_thread_safe.c
index 50226dd..25bbf0b 100644
--- a/program/src/rrd_not_thread_safe.c
+++ b/program/src/rrd_not_thread_safe.c
@@ -14,18 +14,12 @@
#define MAXLEN 4096
#define ERRBUFLEN 256
-static char rrd_error[MAXLEN + 10];
-static char rrd_liberror[ERRBUFLEN + 10];
-static int rrd_context_init = 0;
-
/* The global context is very useful in the transition period to even
more thread-safe stuff, it can be used whereever we need a context
and do not need to worry about concurrency. */
static struct rrd_context global_ctx = {
- MAXLEN,
- ERRBUFLEN,
- rrd_error,
- rrd_liberror
+ "",
+ ""
};
/* #include <stdarg.h> */
@@ -33,11 +27,6 @@ static struct rrd_context global_ctx = {
struct rrd_context *rrd_get_context(
void)
{
- if (!rrd_context_init) {
- rrd_context_init = 1;
- global_ctx.rrd_error[0] = '\0';
- global_ctx.lib_errstr[0] = '\0';
- }
return &global_ctx;
}
diff --git a/program/src/rrd_thread_safe.c b/program/src/rrd_thread_safe.c
index 78164e3..324d468 100644
--- a/program/src/rrd_thread_safe.c
+++ b/program/src/rrd_thread_safe.c
@@ -60,7 +60,7 @@ const char *rrd_strerror(
{
struct rrd_context *ctx = rrd_get_context();
- if (strerror_r(err, ctx->lib_errstr, ctx->errlen))
+ if (strerror_r(err, ctx->lib_errstr, sizeof(ctx->lib_errstr)))
return "strerror_r failed. sorry!";
else
return ctx->lib_errstr;
@@ -75,8 +75,8 @@ const char *rrd_strerror(
ctx = rrd_get_context();
pthread_mutex_lock(&mtx);
- strncpy(ctx->lib_errstr, strerror(err), ctx->errlen);
- ctx->lib_errstr[ctx->errlen] = '\0';
+ strncpy(ctx->lib_errstr, strerror(err), sizeof(ctx->lib_errstr));
+ ctx->lib_errstr[sizeof(ctx->lib_errstr) - 1] = '\0';
pthread_mutex_unlock(&mtx);
return ctx->lib_errstr;
}
diff --git a/program/src/rrd_thread_safe_nt.c b/program/src/rrd_thread_safe_nt.c
index 4faa995..d489985 100644
--- a/program/src/rrd_thread_safe_nt.c
+++ b/program/src/rrd_thread_safe_nt.c
@@ -70,8 +70,8 @@ const char *rrd_strerror(
ctx = rrd_get_context();
EnterCriticalSection(&CriticalSection);
- strncpy(ctx->lib_errstr, strerror(err), ctx->errlen);
- ctx->lib_errstr[ctx->errlen] = '\0';
+ strncpy(ctx->lib_errstr, strerror(err), sizeof(ctx->lib_errstr));
+ ctx->lib_errstr[sizeof(ctx->lib_errstr) - 1] = '\0';
LeaveCriticalSection(&CriticalSection);
return ctx->lib_errstr;
--
1.5.5.1.316.g377d9
signature.asc
Description: Digital signature
