[PATCH] fixes for ctdb/server/eventscript.c
Volker Lendecke
Volker.Lendecke at SerNet.DE
Fri Mar 13 08:27:29 MDT 2015
Hi!
Attached find some fixes for $SUBJECT. I believe we have a
real memleak when collecting scripts. We don't have a crash
or security problem by using strcpy, because we filtered out
long script names before. But still I would argue the
current code is cleaner and more obvious. strcpy raises
eyebrows :-)
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From be716c09fa8fb3a59203617b71458df0f0daf6a3 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Fri, 13 Mar 2015 14:01:25 +0000
Subject: [PATCH 1/5] ctdb: Fix whitespace
Signed-off-by: Volker Lendecke <vl at samba.org>
---
ctdb/server/eventscript.c | 35 +++++++++++++++++------------------
1 file changed, 17 insertions(+), 18 deletions(-)
diff --git a/ctdb/server/eventscript.c b/ctdb/server/eventscript.c
index e70db79..b64808a 100644
--- a/ctdb/server/eventscript.c
+++ b/ctdb/server/eventscript.c
@@ -1,4 +1,4 @@
-/*
+/*
event script handling
Copyright (C) Andrew Tridgell 2007
@@ -7,12 +7,12 @@
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 3 of the License, or
(at your option) any later version.
-
+
This program is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.
-
+
You should have received a copy of the GNU General Public License
along with this program; if not, see <http://www.gnu.org/licenses/>.
*/
@@ -38,7 +38,6 @@ struct event_script_callback {
void (*fn)(struct ctdb_context *, int, void *);
void *private_data;
};
-
struct ctdb_event_script_state {
struct ctdb_context *ctdb;
@@ -48,7 +47,7 @@ struct ctdb_event_script_state {
enum ctdb_eventscript_call call;
const char *options;
struct timeval timeout;
-
+
unsigned int current;
struct ctdb_scripts_wire *scripts;
};
@@ -162,7 +161,7 @@ static struct ctdb_scripts_wire *ctdb_get_script_list(struct ctdb_context *ctdb,
struct ctdb_scripts_wire *scripts;
int count;
- /* scan all directory entries and insert all valid scripts into the
+ /* scan all directory entries and insert all valid scripts into the
tree
*/
count = scandir(ctdb->event_script_dir, &namelist, script_filter, alphasort);
@@ -347,10 +346,10 @@ static int script_status(struct ctdb_scripts_wire *scripts)
}
/* called when child is finished */
-static void ctdb_event_script_handler(struct event_context *ev, struct fd_event *fde,
+static void ctdb_event_script_handler(struct event_context *ev, struct fd_event *fde,
uint16_t flags, void *p)
{
- struct ctdb_event_script_state *state =
+ struct ctdb_event_script_state *state =
talloc_get_type(p, struct ctdb_event_script_state);
struct ctdb_script_wire *current = get_current_script(state);
struct ctdb_context *ctdb = state->ctdb;
@@ -508,7 +507,7 @@ static void ctdb_run_debug_hung_script(struct ctdb_context *ctdb, struct debug_h
}
/* called when child times out */
-static void ctdb_event_script_timeout(struct event_context *ev, struct timed_event *te,
+static void ctdb_event_script_timeout(struct event_context *ev, struct timed_event *te,
struct timeval t, void *p)
{
struct ctdb_event_script_state *state = talloc_get_type(p, struct ctdb_event_script_state);
@@ -610,7 +609,7 @@ static int event_script_destructor(struct ctdb_event_script_state *state)
break;
}
}
-
+
state->callback = NULL;
if (callback) {
@@ -670,7 +669,7 @@ static int remove_callback(struct event_script_callback *callback)
}
/*
- run the event script in the background, calling the callback when
+ run the event script in the background, calling the callback when
finished
*/
static int ctdb_event_script_callback_v(struct ctdb_context *ctdb,
@@ -807,10 +806,10 @@ static int ctdb_event_script_callback_v(struct ctdb_context *ctdb,
/*
- run the event script in the background, calling the callback when
+ run the event script in the background, calling the callback when
finished. If mem_ctx is freed, callback will never be called.
*/
-int ctdb_event_script_callback(struct ctdb_context *ctdb,
+int ctdb_event_script_callback(struct ctdb_context *ctdb,
TALLOC_CTX *mem_ctx,
void (*callback)(struct ctdb_context *, int, void *),
void *private_data,
@@ -895,10 +894,10 @@ struct eventscript_callback_state {
/*
called when a forced eventscript run has finished
*/
-static void run_eventscripts_callback(struct ctdb_context *ctdb, int status,
+static void run_eventscripts_callback(struct ctdb_context *ctdb, int status,
void *private_data)
{
- struct eventscript_callback_state *state =
+ struct eventscript_callback_state *state =
talloc_get_type(private_data, struct eventscript_callback_state);
if (status != 0) {
@@ -964,7 +963,7 @@ int32_t ctdb_run_eventscripts(struct ctdb_context *ctdb,
DEBUG(DEBUG_NOTICE,("Running eventscripts with arguments %s\n", indata.dptr));
- ret = ctdb_event_script_callback(ctdb,
+ ret = ctdb_event_script_callback(ctdb,
state, run_eventscripts_callback, state,
call, "%s", options);
@@ -1007,7 +1006,7 @@ int32_t ctdb_control_enable_script(struct ctdb_context *ctdb, TDB_DATA indata)
}
- if (stat(ctdb->event_script_dir, &st) != 0 &&
+ if (stat(ctdb->event_script_dir, &st) != 0 &&
errno == ENOENT) {
DEBUG(DEBUG_CRIT,("No event script directory found at '%s'\n", ctdb->event_script_dir));
talloc_free(tmp_ctx);
@@ -1063,7 +1062,7 @@ int32_t ctdb_control_disable_script(struct ctdb_context *ctdb, TDB_DATA indata)
}
- if (stat(ctdb->event_script_dir, &st) != 0 &&
+ if (stat(ctdb->event_script_dir, &st) != 0 &&
errno == ENOENT) {
DEBUG(DEBUG_CRIT,("No event script directory found at '%s'\n", ctdb->event_script_dir));
talloc_free(tmp_ctx);
--
1.7.9.5
From 73a8f5754cbf8ac6d821480c07445ec034e0c7a2 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Fri, 13 Mar 2015 14:11:20 +0000
Subject: [PATCH 2/5] ctdb: Make for-loop in ctdb_get_script_list more
idiomatic
Signed-off-by: Volker Lendecke <vl at samba.org>
---
ctdb/server/eventscript.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/ctdb/server/eventscript.c b/ctdb/server/eventscript.c
index b64808a..4cbb846 100644
--- a/ctdb/server/eventscript.c
+++ b/ctdb/server/eventscript.c
@@ -159,7 +159,7 @@ static struct ctdb_scripts_wire *ctdb_get_script_list(struct ctdb_context *ctdb,
{
struct dirent **namelist;
struct ctdb_scripts_wire *scripts;
- int count;
+ int i, count;
/* scan all directory entries and insert all valid scripts into the
tree
@@ -182,11 +182,12 @@ static struct ctdb_scripts_wire *ctdb_get_script_list(struct ctdb_context *ctdb,
}
scripts->num_scripts = count;
- for (count = 0; count < scripts->num_scripts; count++) {
- strcpy(scripts->scripts[count].name, namelist[count]->d_name);
- scripts->scripts[count].status = 0;
- if (!check_executable(ctdb->event_script_dir, namelist[count]->d_name)) {
- scripts->scripts[count].status = -errno;
+ for (i = 0; i < count; i++) {
+ strcpy(scripts->scripts[i].name, namelist[i]->d_name);
+ scripts->scripts[i].status = 0;
+ if (!check_executable(ctdb->event_script_dir,
+ namelist[i]->d_name)) {
+ scripts->scripts[i].status = -errno;
}
}
--
1.7.9.5
From a2154703bde718fa4f9d0dce5556d48dfffcb51b Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Fri, 13 Mar 2015 14:12:41 +0000
Subject: [PATCH 3/5] ctdb: Fix memleak in ctdb_get_script_list
scandir allocates every name individually, see example code in susv4 or man
scandir
Signed-off-by: Volker Lendecke <vl at samba.org>
---
ctdb/server/eventscript.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/ctdb/server/eventscript.c b/ctdb/server/eventscript.c
index 4cbb846..a47396a 100644
--- a/ctdb/server/eventscript.c
+++ b/ctdb/server/eventscript.c
@@ -177,8 +177,7 @@ static struct ctdb_scripts_wire *ctdb_get_script_list(struct ctdb_context *ctdb,
+ sizeof(scripts->scripts[0]) * count);
if (scripts == NULL) {
DEBUG(DEBUG_ERR, (__location__ " Failed to allocate scripts\n"));
- free(namelist);
- return NULL;
+ goto done;
}
scripts->num_scripts = count;
@@ -191,6 +190,10 @@ static struct ctdb_scripts_wire *ctdb_get_script_list(struct ctdb_context *ctdb,
}
}
+done:
+ for (i=0; i<count; i++) {
+ free(namelist[i]);
+ }
free(namelist);
return scripts;
}
--
1.7.9.5
From ab7a99375fa4fd424d979ccc9f55a171159229f9 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Fri, 13 Mar 2015 14:16:17 +0000
Subject: [PATCH 4/5] ctdb: Introduce a helper var in ctdb_get_script_list
Signed-off-by: Volker Lendecke <vl at samba.org>
---
ctdb/server/eventscript.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/ctdb/server/eventscript.c b/ctdb/server/eventscript.c
index a47396a..b51d953 100644
--- a/ctdb/server/eventscript.c
+++ b/ctdb/server/eventscript.c
@@ -182,11 +182,13 @@ static struct ctdb_scripts_wire *ctdb_get_script_list(struct ctdb_context *ctdb,
scripts->num_scripts = count;
for (i = 0; i < count; i++) {
- strcpy(scripts->scripts[i].name, namelist[i]->d_name);
- scripts->scripts[i].status = 0;
+ struct ctdb_script_wire *s = &scripts->scripts[i];
+
+ strcpy(s->name, namelist[i]->d_name);
+ s->status = 0;
if (!check_executable(ctdb->event_script_dir,
namelist[i]->d_name)) {
- scripts->scripts[i].status = -errno;
+ s->status = -errno;
}
}
--
1.7.9.5
From 36232e0ec01a2617603a2f90903d8cdadb69bc81 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Fri, 13 Mar 2015 14:20:05 +0000
Subject: [PATCH 5/5] ctdb: Fix 1125613 Destination buffer too small
Signed-off-by: Volker Lendecke <vl at samba.org>
---
ctdb/server/eventscript.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/ctdb/server/eventscript.c b/ctdb/server/eventscript.c
index b51d953..4dd4379 100644
--- a/ctdb/server/eventscript.c
+++ b/ctdb/server/eventscript.c
@@ -184,7 +184,12 @@ static struct ctdb_scripts_wire *ctdb_get_script_list(struct ctdb_context *ctdb,
for (i = 0; i < count; i++) {
struct ctdb_script_wire *s = &scripts->scripts[i];
- strcpy(s->name, namelist[i]->d_name);
+ if (strlcpy(s->name, namelist[i]->d_name, sizeof(s->name)) >=
+ sizeof(s->name)) {
+ s->status = -ENAMETOOLONG;
+ continue;
+ }
+
s->status = 0;
if (!check_executable(ctdb->event_script_dir,
namelist[i]->d_name)) {
@@ -335,6 +340,7 @@ static int script_status(struct ctdb_scripts_wire *scripts)
for (i = 0; i < scripts->num_scripts; i++) {
switch (scripts->scripts[i].status) {
+ case -ENAMETOOLONG:
case -ENOENT:
case -ENOEXEC:
/* Disabled or missing; that's OK. */
--
1.7.9.5
More information about the samba-technical
mailing list