[PATCH] small refactorings to smbXsrv_open

Jeremy Allison jra at samba.org
Fri Feb 26 18:26:29 UTC 2016


On Fri, Feb 26, 2016 at 04:56:50PM +0100, Michael Adam wrote:
> On 2016-02-26 at 16:28 +0100, Volker Lendecke wrote:
> > On Fri, Feb 26, 2016 at 04:20:05PM +0100, Michael Adam wrote:
> > > Currently no, I guess I added it because I always
> > > feel a little uneasy when I add such a function
> > > where a caller could trigger a null deref.
> > > Call me paranoid. ;-)
> > 
> > With these checks in place I immediately look for the circumstances when
> > "db" can be NULL.
> 
> Heh, my reflex is to start verifying that all callers pass db != NULL
> if I see a function withOUT such checks. ;-)
> 
> > If in the future this needs to be legitimately calleable
> > without a db, then we can add it. But for that we need to really make
> > sure we understand why we need a "db fetch" call without a db in the
> > first place.
> 
> Updated patchset attached.

Nice cleanup Michael - thanks ! Especially for labelling
the TALLOC_CTX parameters when they look indistinguishable
from one of the other params :-).

Pushed !

> I'll also send corresponding patches for smbXsrv_session and
> _tcon as requested by Metze.
> 
> Michael

> From 9c472316b1e86cb6d3e40c3b5012d7a28cd97456 Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Fri, 26 Feb 2016 00:41:24 +0100
> Subject: [PATCH 1/2] smbXsrv_open: factor fetch-locking of global record into
>  function
> 
> smbXsrv_open_global_fetch_locked()
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> ---
>  source3/smbd/smbXsrv_open.c | 85 +++++++++++++++++++--------------------------
>  1 file changed, 36 insertions(+), 49 deletions(-)
> 
> diff --git a/source3/smbd/smbXsrv_open.c b/source3/smbd/smbXsrv_open.c
> index 58b0dfa..e8952db 100644
> --- a/source3/smbd/smbXsrv_open.c
> +++ b/source3/smbd/smbXsrv_open.c
> @@ -151,6 +151,27 @@ static NTSTATUS smbXsrv_open_local_key_to_id(TDB_DATA key, uint32_t *id)
>  	return NT_STATUS_OK;
>  }
>  
> +static struct db_record *smbXsrv_open_global_fetch_locked(
> +			struct db_context *db,
> +			uint32_t id,
> +			TALLOC_CTX *mem_ctx)
> +{
> +	TDB_DATA key;
> +	uint8_t key_buf[SMBXSRV_OPEN_GLOBAL_TDB_KEY_SIZE];
> +	struct db_record *rec = NULL;
> +
> +	key = smbXsrv_open_global_id_to_key(id, key_buf);
> +
> +	rec = dbwrap_fetch_locked(db, mem_ctx, key);
> +
> +	if (rec == NULL) {
> +		DBG_DEBUG("Failed to lock global id 0x%08x, key '%s'\n", id,
> +			  hex_encode_talloc(talloc_tos(), key.dptr, key.dsize));
> +	}
> +
> +	return rec;
> +}
> +
>  static NTSTATUS smbXsrv_open_table_init(struct smbXsrv_connection *conn,
>  					uint32_t lowest_id,
>  					uint32_t highest_id,
> @@ -497,8 +518,6 @@ static NTSTATUS smbXsrv_open_global_allocate(struct db_context *db,
>  		bool is_free = false;
>  		bool was_free = false;
>  		uint32_t id;
> -		uint8_t key_buf[SMBXSRV_OPEN_GLOBAL_TDB_KEY_SIZE];
> -		TDB_DATA key;
>  
>  		if (i >= min_tries && last_free != 0) {
>  			id = last_free;
> @@ -512,9 +531,7 @@ static NTSTATUS smbXsrv_open_global_allocate(struct db_context *db,
>  			id--;
>  		}
>  
> -		key = smbXsrv_open_global_id_to_key(id, key_buf);
> -
> -		global->db_rec = dbwrap_fetch_locked(db, mem_ctx, key);
> +		global->db_rec = smbXsrv_open_global_fetch_locked(db, id, mem_ctx);
>  		if (global->db_rec == NULL) {
>  			talloc_free(global);
>  			return NT_STATUS_INSUFFICIENT_RESOURCES;
> @@ -720,8 +737,6 @@ static NTSTATUS smbXsrv_open_global_lookup(struct smbXsrv_open_table *table,
>  					   TALLOC_CTX *mem_ctx,
>  					   struct smbXsrv_open_global0 **_global)
>  {
> -	TDB_DATA key;
> -	uint8_t key_buf[SMBXSRV_OPEN_GLOBAL_TDB_KEY_SIZE];
>  	struct db_record *global_rec = NULL;
>  	bool is_free = false;
>  
> @@ -731,15 +746,10 @@ static NTSTATUS smbXsrv_open_global_lookup(struct smbXsrv_open_table *table,
>  		return NT_STATUS_INTERNAL_ERROR;
>  	}
>  
> -	key = smbXsrv_open_global_id_to_key(open_global_id, key_buf);
> -
> -	global_rec = dbwrap_fetch_locked(table->global.db_ctx, mem_ctx, key);
> +	global_rec = smbXsrv_open_global_fetch_locked(table->global.db_ctx,
> +						      open_global_id,
> +						      mem_ctx);
>  	if (global_rec == NULL) {
> -		DEBUG(0, ("smbXsrv_open_global_lookup(0x%08x): "
> -			  "Failed to lock global key '%s'\n",
> -			  open_global_id,
> -			  hex_encode_talloc(talloc_tos(), key.dptr,
> -					    key.dsize)));
>  		return NT_STATUS_INTERNAL_DB_ERROR;
>  	}
>  
> @@ -912,8 +922,6 @@ NTSTATUS smbXsrv_open_update(struct smbXsrv_open *op)
>  {
>  	struct smbXsrv_open_table *table = op->table;
>  	NTSTATUS status;
> -	uint8_t key_buf[SMBXSRV_OPEN_GLOBAL_TDB_KEY_SIZE];
> -	TDB_DATA key;
>  
>  	if (op->global->db_rec != NULL) {
>  		DEBUG(0, ("smbXsrv_open_update(0x%08x): "
> @@ -922,17 +930,11 @@ NTSTATUS smbXsrv_open_update(struct smbXsrv_open *op)
>  		return NT_STATUS_INTERNAL_ERROR;
>  	}
>  
> -	key = smbXsrv_open_global_id_to_key(op->global->open_global_id,
> -					    key_buf);
> -
> -	op->global->db_rec = dbwrap_fetch_locked(table->global.db_ctx,
> -						 op->global, key);
> +	op->global->db_rec = smbXsrv_open_global_fetch_locked(
> +						table->global.db_ctx,
> +						op->global->open_global_id,
> +						op->global /* TALLOC_CTX */);
>  	if (op->global->db_rec == NULL) {
> -		DEBUG(0, ("smbXsrv_open_update(0x%08x): "
> -			  "Failed to lock global key '%s'\n",
> -			  op->global->open_global_id,
> -			  hex_encode_talloc(talloc_tos(), key.dptr,
> -					    key.dsize)));
>  		return NT_STATUS_INTERNAL_DB_ERROR;
>  	}
>  
> @@ -982,21 +984,11 @@ NTSTATUS smbXsrv_open_close(struct smbXsrv_open *op, NTTIME now)
>  	global_rec = op->global->db_rec;
>  	op->global->db_rec = NULL;
>  	if (global_rec == NULL) {
> -		uint8_t key_buf[SMBXSRV_OPEN_GLOBAL_TDB_KEY_SIZE];
> -		TDB_DATA key;
> -
> -		key = smbXsrv_open_global_id_to_key(
> -						op->global->open_global_id,
> -						key_buf);
> -
> -		global_rec = dbwrap_fetch_locked(table->global.db_ctx,
> -						 op->global, key);
> +		global_rec = smbXsrv_open_global_fetch_locked(
> +					table->global.db_ctx,
> +					op->global->open_global_id,
> +					op->global /* TALLOC_CTX */);
>  		if (global_rec == NULL) {
> -			DEBUG(0, ("smbXsrv_open_close(0x%08x): "
> -				  "Failed to lock global key '%s'\n",
> -				  op->global->open_global_id,
> -				  hex_encode_talloc(global_rec, key.dptr,
> -						    key.dsize)));
>  			error = NT_STATUS_INTERNAL_ERROR;
>  		}
>  	}
> @@ -1406,21 +1398,16 @@ NTSTATUS smbXsrv_open_cleanup(uint64_t persistent_id)
>  	NTSTATUS status = NT_STATUS_OK;
>  	TALLOC_CTX *frame = talloc_stackframe();
>  	struct smbXsrv_open_global0 *op = NULL;
> -	uint8_t key_buf[SMBXSRV_OPEN_GLOBAL_TDB_KEY_SIZE];
> -	TDB_DATA key;
>  	TDB_DATA val;
>  	struct db_record *rec;
>  	bool delete_open = false;
>  	uint32_t global_id = persistent_id & UINT32_MAX;
>  
> -	key = smbXsrv_open_global_id_to_key(global_id, key_buf);
> -	rec = dbwrap_fetch_locked(smbXsrv_open_global_db_ctx, frame, key);
> +	rec = smbXsrv_open_global_fetch_locked(smbXsrv_open_global_db_ctx,
> +					       global_id,
> +					       frame);
>  	if (rec == NULL) {
>  		status = NT_STATUS_NOT_FOUND;
> -		DEBUG(1, ("smbXsrv_open_cleanup[global: 0x%08x] "
> -			  "failed to fetch record from %s - %s\n",
> -			   global_id, dbwrap_name(smbXsrv_open_global_db_ctx),
> -			   nt_errstr(status)));
>  		goto done;
>  	}
>  
> -- 
> 2.5.0
> 
> 
> From 278b9b139f26bfa54ce99086ee1b2ee0a2f3c00d Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Fri, 26 Feb 2016 00:53:22 +0100
> Subject: [PATCH 2/2] smbXsrv_open: factor fetch-locking of local record into
>  function
> 
> smbXsrv_open_local_fetch_locked()
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> ---
>  source3/smbd/smbXsrv_open.c | 48 ++++++++++++++++++++++++---------------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/source3/smbd/smbXsrv_open.c b/source3/smbd/smbXsrv_open.c
> index e8952db..705c988 100644
> --- a/source3/smbd/smbXsrv_open.c
> +++ b/source3/smbd/smbXsrv_open.c
> @@ -172,6 +172,27 @@ static struct db_record *smbXsrv_open_global_fetch_locked(
>  	return rec;
>  }
>  
> +static struct db_record *smbXsrv_open_local_fetch_locked(
> +			struct db_context *db,
> +			uint32_t id,
> +			TALLOC_CTX *mem_ctx)
> +{
> +	TDB_DATA key;
> +	uint8_t key_buf[SMBXSRV_OPEN_LOCAL_TDB_KEY_SIZE];
> +	struct db_record *rec = NULL;
> +
> +	key = smbXsrv_open_local_id_to_key(id, key_buf);
> +
> +	rec = dbwrap_fetch_locked(db, mem_ctx, key);
> +
> +	if (rec == NULL) {
> +		DBG_DEBUG("Failed to lock local id 0x%08x, key '%s'\n", id,
> +			  hex_encode_talloc(talloc_tos(), key.dptr, key.dsize));
> +	}
> +
> +	return rec;
> +}
> +
>  static NTSTATUS smbXsrv_open_table_init(struct smbXsrv_connection *conn,
>  					uint32_t lowest_id,
>  					uint32_t highest_id,
> @@ -296,8 +317,6 @@ static NTSTATUS smbXsrv_open_local_allocate_id(struct db_context *db,
>  
>  	for (i = 0; i < (range / 2); i++) {
>  		uint32_t id;
> -		uint8_t key_buf[SMBXSRV_OPEN_LOCAL_TDB_KEY_SIZE];
> -		TDB_DATA key;
>  		TDB_DATA val;
>  		struct db_record *rec = NULL;
>  
> @@ -311,9 +330,7 @@ static NTSTATUS smbXsrv_open_local_allocate_id(struct db_context *db,
>  			id = highest_id;
>  		}
>  
> -		key = smbXsrv_open_local_id_to_key(id, key_buf);
> -
> -		rec = dbwrap_fetch_locked(db, mem_ctx, key);
> +		rec = smbXsrv_open_local_fetch_locked(db, id, mem_ctx);
>  		if (rec == NULL) {
>  			return NT_STATUS_INSUFFICIENT_RESOURCES;
>  		}
> @@ -363,16 +380,12 @@ static NTSTATUS smbXsrv_open_local_allocate_id(struct db_context *db,
>  
>  	if (NT_STATUS_IS_OK(state.status)) {
>  		uint32_t id;
> -		uint8_t key_buf[SMBXSRV_OPEN_LOCAL_TDB_KEY_SIZE];
> -		TDB_DATA key;
>  		TDB_DATA val;
>  		struct db_record *rec = NULL;
>  
>  		id = state.useable_id;
>  
> -		key = smbXsrv_open_local_id_to_key(id, key_buf);
> -
> -		rec = dbwrap_fetch_locked(db, mem_ctx, key);
> +		rec = smbXsrv_open_local_fetch_locked(db, id, mem_ctx);
>  		if (rec == NULL) {
>  			return NT_STATUS_INSUFFICIENT_RESOURCES;
>  		}
> @@ -1047,19 +1060,10 @@ NTSTATUS smbXsrv_open_close(struct smbXsrv_open *op, NTTIME now)
>  
>  	local_rec = op->db_rec;
>  	if (local_rec == NULL) {
> -		uint8_t key_buf[SMBXSRV_OPEN_LOCAL_TDB_KEY_SIZE];
> -		TDB_DATA key;
> -
> -		key = smbXsrv_open_local_id_to_key(op->local_id, key_buf);
> -
> -		local_rec = dbwrap_fetch_locked(table->local.db_ctx,
> -						op, key);
> +		local_rec = smbXsrv_open_local_fetch_locked(table->local.db_ctx,
> +							    op->local_id,
> +							    op /* TALLOC_CTX*/);
>  		if (local_rec == NULL) {
> -			DEBUG(0, ("smbXsrv_open_close(0x%08x): "
> -				  "Failed to lock local key '%s'\n",
> -				  op->global->open_global_id,
> -				  hex_encode_talloc(local_rec, key.dptr,
> -						    key.dsize)));
>  			error = NT_STATUS_INTERNAL_ERROR;
>  		}
>  	}
> -- 
> 2.5.0
> 






More information about the samba-technical mailing list