[PATCH 1/2] tdb: don't corrupt database if we go overlength due to transaction expand.
Volker Lendecke
Volker.Lendecke at SerNet.DE
Tue May 28 06:21:31 MDT 2013
Hi, Rusty!
On Tue, May 28, 2013 at 10:18:37AM +0200, Volker Lendecke wrote:
> Ignore me. Sorry for the noise.
Attached find a different patchset where Stefan and myself
went through the tdb_expand routine and (as we believe)
fixed some more overflow conditions. I need to take a
further look at the transaction stuff, but I do not want
this to get lost.
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 8313a101525cb089017dc9dad968b8f696a07065 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 28 May 2013 12:56:57 +0200
Subject: [PATCH 1/5] tdb: add a 'new_size' helper variable to tdb_expand_file()
Pair-Programmed-With: Volker Lendecke <vl at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Volker Lendecke <vl at samba.org>
---
lib/tdb/common/io.c | 11 +++++++----
1 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/lib/tdb/common/io.c b/lib/tdb/common/io.c
index a477fb5..44ef728 100644
--- a/lib/tdb/common/io.c
+++ b/lib/tdb/common/io.c
@@ -287,18 +287,21 @@ int tdb_mmap(struct tdb_context *tdb)
static int tdb_expand_file(struct tdb_context *tdb, tdb_off_t size, tdb_off_t addition)
{
char buf[8192];
+ tdb_off_t new_size;
if (tdb->read_only || tdb->traverse_read) {
tdb->ecode = TDB_ERR_RDONLY;
return -1;
}
- if (ftruncate(tdb->fd, size+addition) == -1) {
+ new_size = size + addition;
+
+ if (ftruncate(tdb->fd, new_size) == -1) {
char b = 0;
- ssize_t written = pwrite(tdb->fd, &b, 1, (size+addition) - 1);
+ ssize_t written = pwrite(tdb->fd, &b, 1, new_size - 1);
if (written == 0) {
/* try once more, potentially revealing errno */
- written = pwrite(tdb->fd, &b, 1, (size+addition) - 1);
+ written = pwrite(tdb->fd, &b, 1, new_size - 1);
}
if (written == 0) {
/* again - give up, guessing errno */
@@ -306,7 +309,7 @@ static int tdb_expand_file(struct tdb_context *tdb, tdb_off_t size, tdb_off_t ad
}
if (written != 1) {
TDB_LOG((tdb, TDB_DEBUG_FATAL, "expand_file to %u failed (%s)\n",
- size+addition, strerror(errno)));
+ (unsigned)new_size, strerror(errno)));
return -1;
}
}
--
1.7.3.4
From 73de3aa3fc1ff7698e063de738e3f8df81560b0a Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 28 May 2013 12:59:32 +0200
Subject: [PATCH 2/5] tdb: add overflow/ENOSPC handling to tdb_expand_file()
Pair-Programmed-With: Volker Lendecke <vl at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Volker Lendecke <vl at samba.org>
---
lib/tdb/common/io.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/lib/tdb/common/io.c b/lib/tdb/common/io.c
index 44ef728..53f7030 100644
--- a/lib/tdb/common/io.c
+++ b/lib/tdb/common/io.c
@@ -295,6 +295,15 @@ static int tdb_expand_file(struct tdb_context *tdb, tdb_off_t size, tdb_off_t ad
}
new_size = size + addition;
+ if ((new_size < size) || (new_size < addition)) {
+ /* overflow */
+ tdb->ecode = TDB_ERR_OOM;
+ TDB_LOG((tdb, TDB_DEBUG_FATAL, "expand_file write "
+ "overflow detected current size[%u] addition[%u]!\n",
+ (unsigned)size, (unsigned)addition));
+ errno = ENOSPC;
+ return -1;
+ }
if (ftruncate(tdb->fd, new_size) == -1) {
char b = 0;
@@ -308,6 +317,7 @@ static int tdb_expand_file(struct tdb_context *tdb, tdb_off_t size, tdb_off_t ad
errno = ENOSPC;
}
if (written != 1) {
+ tdb->ecode = TDB_ERR_OOM;
TDB_LOG((tdb, TDB_DEBUG_FATAL, "expand_file to %u failed (%s)\n",
(unsigned)new_size, strerror(errno)));
return -1;
@@ -327,12 +337,14 @@ static int tdb_expand_file(struct tdb_context *tdb, tdb_off_t size, tdb_off_t ad
}
if (written == 0) {
/* give up, trying to provide a useful errno */
+ tdb->ecode = TDB_ERR_OOM;
TDB_LOG((tdb, TDB_DEBUG_FATAL, "expand_file write "
"returned 0 twice: giving up!\n"));
errno = ENOSPC;
return -1;
}
if (written == -1) {
+ tdb->ecode = TDB_ERR_OOM;
TDB_LOG((tdb, TDB_DEBUG_FATAL, "expand_file write of "
"%u bytes failed (%s)\n", (int)n,
strerror(errno)));
--
1.7.3.4
From 5e1bc63b9444e730c9c17b4a4d3e47e835c2d92e Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 28 May 2013 13:01:27 +0200
Subject: [PATCH 3/5] tdb: add overflow detection to tdb_expand_adjust()
We round up at maximun to a new size of 4GB,
but still return at least the given size.
The caller has to deal with ENOSPC itself.
Pair-Programmed-With: Volker Lendecke <vl at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Volker Lendecke <vl at samba.org>
---
lib/tdb/common/io.c | 35 ++++++++++++++++++++++++++++++++---
1 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/lib/tdb/common/io.c b/lib/tdb/common/io.c
index 53f7030..01768bc 100644
--- a/lib/tdb/common/io.c
+++ b/lib/tdb/common/io.c
@@ -365,14 +365,33 @@ static int tdb_expand_file(struct tdb_context *tdb, tdb_off_t size, tdb_off_t ad
/* You need 'size', this tells you how much you should expand by. */
tdb_off_t tdb_expand_adjust(tdb_off_t map_size, tdb_off_t size, int page_size)
{
- tdb_off_t new_size, top_size;
+ tdb_off_t new_size, top_size, increment;
+ tdb_off_t max_size = UINT32_MAX - map_size;
+
+ if (size > max_size) {
+ /*
+ * We can't round up anymore, just give back
+ * what we're asked for.
+ *
+ * The caller has to take care of the ENOSPC handling.
+ */
+ return size;
+ }
/* limit size in order to avoid using up huge amounts of memory for
* in memory tdbs if an oddball huge record creeps in */
if (size > 100 * 1024) {
- top_size = map_size + size * 2;
+ increment = size * 2;
} else {
- top_size = map_size + size * 100;
+ increment = size * 100;
+ }
+ if (increment < size) {
+ goto overflow;
+ }
+
+ top_size = map_size + increment;
+ if ((top_size < map_size) || (top_size < increment)) {
+ goto overflow;
}
/* always make room for at least top_size more records, and at
@@ -383,10 +402,20 @@ tdb_off_t tdb_expand_adjust(tdb_off_t map_size, tdb_off_t size, int page_size)
} else {
new_size = map_size * 1.25;
}
+ if (new_size < map_size) {
+ goto overflow;
+ }
/* Round the database up to a multiple of the page size */
new_size = MAX(top_size, new_size);
return TDB_ALIGN(new_size, page_size) - map_size;
+
+overflow:
+ /*
+ * Somewhere in between we went over 4GB. Make one big jump to
+ * exactly 4GB database size.
+ */
+ return max_size;
}
/* expand the database at least size bytes by expanding the underlying
--
1.7.3.4
From 6fc383c94c7d40cb6512892ffd288bccd0875bd2 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 28 May 2013 13:04:29 +0200
Subject: [PATCH 4/5] tdb: add proper OOM/ENOSPC handling to tdb_expand()
Failing to do so will result in corrupt tdbs: We will overwrite
the hash chain pointers with 0x42424242.
Pair-Programmed-With: Volker Lendecke <vl at samba.org>
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Volker Lendecke <vl at samba.org>
---
lib/tdb/common/io.c | 32 ++++++++++++++++++++++++--------
1 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/lib/tdb/common/io.c b/lib/tdb/common/io.c
index 01768bc..f6bf372 100644
--- a/lib/tdb/common/io.c
+++ b/lib/tdb/common/io.c
@@ -424,6 +424,7 @@ int tdb_expand(struct tdb_context *tdb, tdb_off_t size)
{
struct tdb_record rec;
tdb_off_t offset;
+ tdb_off_t new_size;
if (tdb_lock(tdb, -1, F_WRLCK) == -1) {
TDB_LOG((tdb, TDB_DEBUG_ERROR, "lock failed in tdb_expand\n"));
@@ -435,10 +436,13 @@ int tdb_expand(struct tdb_context *tdb, tdb_off_t size)
size = tdb_expand_adjust(tdb->map_size, size, tdb->page_size);
- /* expand the file itself */
- if (!(tdb->flags & TDB_INTERNAL)) {
- if (tdb->methods->tdb_expand_file(tdb, tdb->map_size, size) != 0)
- goto fail;
+ new_size = tdb->map_size + size;
+ if ((new_size < tdb->map_size) || (new_size < size)) {
+ tdb->ecode = TDB_ERR_OOM;
+ TDB_LOG((tdb, TDB_DEBUG_FATAL, "tdb_expand "
+ "overflow detected current map_size[%u] size[%u]!\n",
+ (unsigned)tdb->map_size, (unsigned)size));
+ goto fail;
}
/* form a new freelist record */
@@ -447,18 +451,30 @@ int tdb_expand(struct tdb_context *tdb, tdb_off_t size)
rec.rec_len = size - sizeof(rec);
if (tdb->flags & TDB_INTERNAL) {
- char *new_map_ptr = (char *)realloc(tdb->map_ptr,
- tdb->map_size + size);
+ char *new_map_ptr;
+
+ new_map_ptr = (char *)realloc(tdb->map_ptr, new_size);
if (!new_map_ptr) {
+ tdb->ecode = TDB_ERR_OOM;
goto fail;
}
tdb->map_ptr = new_map_ptr;
- tdb->map_size += size;
+ tdb->map_size = new_size;
} else {
+ int ret;
+
+ /*
+ * expand the file itself
+ */
+ ret = tdb->methods->tdb_expand_file(tdb, tdb->map_size, size);
+ if (ret != 0) {
+ goto fail;
+ }
+
/* Explicitly remap: if we're in a transaction, this won't
* happen automatically! */
tdb_munmap(tdb);
- tdb->map_size += size;
+ tdb->map_size = new_size;
if (tdb_mmap(tdb) != 0) {
goto fail;
}
--
1.7.3.4
From 05ae5548156047377e9ab51c6592e3d193bb1509 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 28 May 2013 13:22:43 +0200
Subject: [PATCH 5/5] TODO: fix tdb_recovery_allocate
---
lib/tdb/common/transaction.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/lib/tdb/common/transaction.c b/lib/tdb/common/transaction.c
index 81cfd16..14bd36c 100644
--- a/lib/tdb/common/transaction.c
+++ b/lib/tdb/common/transaction.c
@@ -743,7 +743,7 @@ static int tdb_recovery_allocate(struct tdb_context *tdb,
*recovery_size,
tdb->page_size)
- sizeof(rec);
-
+//TODO: detect overflow...
new_end = recovery_head + sizeof(rec) + *recovery_max_size;
if (methods->tdb_expand_file(tdb, tdb->transaction->old_map_size,
--
1.7.3.4
More information about the samba-technical
mailing list