[PATCH 1/2] tdb: don't corrupt database if we go overlength due to transaction expand.

[Rusty Russell]

Volker Lendecke <Volker.Lendecke at SerNet.DE> writes:
> 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.

Yes, thanks for the more thorough look.  Comments inline:

> 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(-)

Pretty trivial, but ok.

Reviewed-by: Rusty Russell <rusty at rustcorp.com.au>

>
> 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;
> +	}

This is nice, too.

Reviewed-by: Rusty Russell <rusty at rustcorp.com.au>

>  
>  	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.

I think I prefer a bool return here when we fail.  That has the nice
property of doing our overflow checking in one place.

Hmm, not quite: tdb_recovery_size() could theoretically overflow.  So
could the addition of sizeof(rec).  I think a bool tdb_overflow() helper
might be a good idea:

        bool tdb_overflow(tdb_off_t *ret, tdb_off_t a, tdb_off_t b)
        {
                if (a + b < a)
                        return false;
                *ret = a + b;
                return true;
        }

We can then make tdb_recovery_size() return a bool, too...

Cheers,
Rusty,