[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