[PATCH] small refactorings to smbXsrv_open
Michael Adam
obnox at samba.org
Fri Feb 26 15:56:50 UTC 2016
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.
I'll also send corresponding patches for smbXsrv_session and
_tcon as requested by Metze.
Michael
-------------- next part --------------
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160226/7a376e43/signature.sig>
More information about the samba-technical
mailing list