[PATCH] Fix possible crash bugs in tdb code.

Michael Adam ma at sernet.de
Thu Dec 13 08:26:40 GMT 2007


Andreas Schneider wrote:
> 
> The first patch removes the code from winbind to restart it if the cache is
> invalid. I've talked to Michael Adam and we both think that a restart isn't
> needed anymore as the signal handlers changed.

First patch applied to v3-2-test in aabe9b33fcaed8af98b1ed6b736253e196d87d48.

Thanks - Michael

> The seconds just checks if the winbindd_invalidate_cache() function fails. On
> an error we close the cache revalidate it and initialize it again.
> 
> I've tested it letting winbindd_invalidate_cache() always return -1. From my
> point of view everything worked just fine. Maybe it would be better to restart
> winbind, but then we need a restart function and have to store argv and envp.
> 
> 
> 	-- andreas

> From 8a1810ed21bfa18ed589d43ea6587c09b8ed4d14 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <anschneider at suse.de>
> Date: Fri, 23 Nov 2007 10:54:48 +0100
> Subject: [PATCH] Don't restart winbind if a corrupted tdb is found during initialization.
> 
> The tdb is validated before it gets initialized. Since then sighandlers changed
> a restart isn't needed anymore.
> ---
>  source/winbindd/winbindd.c       |   24 +++++++++---------------
>  source/winbindd/winbindd_cache.c |    3 +--
>  2 files changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/source/winbindd/winbindd.c b/source/winbindd/winbindd.c
> index 4194b55..f685256 100644
> --- a/source/winbindd/winbindd.c
> +++ b/source/winbindd/winbindd.c
> @@ -1159,20 +1159,6 @@ int main(int argc, char **argv, char **envp)
>  
>  	pidfile_create("winbindd");
>  
> -	/* Ensure all cache and idmap caches are consistent
> -	   before we startup. */
> -
> -	if (winbindd_validate_cache()) {
> -		/* We have a bad cache, but luckily we
> -		   just deleted it. Restart ourselves */
> -		int i;
> -		/* Ensure we have no open low fd's. */
> -		for (i = 3; i < 100; i++) {
> -			close(i);
> -		}
> -		return execve(argv[0], argv, envp);
> -	}
> -
>  #if HAVE_SETPGID
>  	/*
>  	 * If we're interactive we want to set our own process group for
> @@ -1190,7 +1176,15 @@ int main(int argc, char **argv, char **envp)
>  		DEBUG(0, ("unable to initialize messaging system\n"));
>  		exit(1);
>  	}
> -	
> +
> +	/*
> +	 * Ensure all cache and idmap caches are consistent
> +	 * before we startup.
> +	 */
> +	if (winbindd_validate_cache() < 0) {
> +		DEBUG(0, ("corrupted tdb found, trying to restore backup\n"));
> +	}
> +
>  	/* Initialize cache (ensure version is correct). */
>  	if (!initialize_winbindd_cache()) {
>  		exit(1);
> diff --git a/source/winbindd/winbindd_cache.c b/source/winbindd/winbindd_cache.c
> index f3aa0fc..7ec8208 100644
> --- a/source/winbindd/winbindd_cache.c
> +++ b/source/winbindd/winbindd_cache.c
> @@ -3338,8 +3338,7 @@ static void validate_panic(const char *const why)
>  
>  /***********************************************************************
>   Try and validate every entry in the winbindd cache. If we fail here,
> - delete the cache tdb and return non-zero - the caller (main winbindd
> - function) will restart us as we don't know if we crashed or not.
> + delete the cache tdb and return non-zero.
>  ***********************************************************************/
>  
>  int winbindd_validate_cache(void)
> -- 
> 1.5.3.4
> 

> From 36670f458fbf5bcd30b12507ee51da1a0a147432 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <anschneider at suse.de>
> Date: Mon, 26 Nov 2007 11:44:30 +0100
> Subject: [PATCH] Prevent winbindd from segfaulting due to corrupted cache tdb.
> 
> If we try to flush the caches and due to a corrupted tdb we and have no tdb
> context close the tdb and validate it. Initialize the cache afterwards again.
> ---
>  source/winbindd/winbindd.c       |   18 +++++++++++++++++-
>  source/winbindd/winbindd_cache.c |   22 +++++++++++++++++++---
>  2 files changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/source/winbindd/winbindd.c b/source/winbindd/winbindd.c
> index f685256..66d3171 100644
> --- a/source/winbindd/winbindd.c
> +++ b/source/winbindd/winbindd.c
> @@ -121,7 +121,23 @@ static void flush_caches(void)
>             otherwise cached access denied errors due to restrict anonymous
>             hang around until the sequence number changes. */
>  
> -	wcache_invalidate_cache();
> +	if (wcache_invalidate_cache() < 0) {
> +		DEBUG(0, ("invalidating the cache failed; revalidate the cache\n"));
> +		/* Close the cache to be able to valdite the cache */
> +		close_winbindd_cache();
> +		/*
> +		 * Ensure all cache and idmap caches are consistent
> +		 * before we initialize the cache again.
> +		 */
> +		if (winbindd_validate_cache() < 0) {
> +			DEBUG(0, ("corrupted tdb found, trying to restore backup\n"));
> +		}
> +
> +		/* Initialize cache again. */
> +		if (!initialize_winbindd_cache()) {
> +			exit(1);
> +		}
> +	}
>  }
>  
>  /* Handle the signal by unlinking socket and exiting */
> diff --git a/source/winbindd/winbindd_cache.c b/source/winbindd/winbindd_cache.c
> index 7ec8208..07b4d46 100644
> --- a/source/winbindd/winbindd_cache.c
> +++ b/source/winbindd/winbindd_cache.c
> @@ -2249,7 +2249,7 @@ void wcache_invalidate_samlogon(struct winbindd_domain *domain,
>  	netsamlogon_clear_cached_user(cache->tdb, info3);
>  }
>  
> -void wcache_invalidate_cache(void)
> +int wcache_invalidate_cache(void)
>  {
>  	struct winbindd_domain *domain;
>  
> @@ -2258,9 +2258,15 @@ void wcache_invalidate_cache(void)
>  
>  		DEBUG(10, ("wcache_invalidate_cache: invalidating cache "
>  			   "entries for %s\n", domain->name));
> -		if (cache)
> -			tdb_traverse(cache->tdb, traverse_fn, NULL);
> +		if (cache) {
> +			if (cache->tdb) {
> +				tdb_traverse(cache->tdb, traverse_fn, NULL);
> +			} else {
> +				return -1;
> +			}
> +		}
>  	}
> +	return 0;
>  }
>  
>  bool init_wcache(void)
> @@ -2342,6 +2348,16 @@ bool initialize_winbindd_cache(void)
>  	return True;
>  }
>  
> +void close_winbindd_cache()
> +{
> +	if (!wcache)
> +		return;
> +	if (wcache->tdb) {
> +		tdb_close(wcache->tdb);
> +		wcache->tdb = NULL;
> +	}
> +}
> +
>  void cache_store_response(pid_t pid, struct winbindd_response *response)
>  {
>  	fstring key_str;
> -- 
> 1.5.3.4



-- 
Michael Adam <ma at sernet.de>
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: Info @ SerNet.DE
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20071213/e58a5f79/attachment.bin


More information about the samba-technical mailing list