playing with composite functions

Peter Novodvorsky nidd at myxomop.com
Mon Feb 28 21:50:27 GMT 2005


Hello!

Lately I've been playing with composite functions from Samba 4. I've
wrote appendacl composite function that opens the file, appends acl to
it's current acl and returns full new acl. In parallel I've added
function security_descriptor_copy function that had FIXME: TODO
previously. 

I've added testing code in torture for this function and found strange
behaviour of talloc. If you look at the code (it is included below) at
appendacl.c:293 you will find security_descriptor_copy function
call. In theory it shouldn't be necessary. I could just talloc_steal
this structure from composite context. However, when I go with
talloc_steal it crashes at composite.c:211. Valgrind says that this
part of memory was freed just a few lines after appendacl.c:293, when
I'm freeing composite function's context from which I've just stolen
this memory. 

I've tried to discuss it on IRC this morning but had no success with
that.

Code is attached in plain text as Alexander Bokovoy suggested.

Peter.

Index: source/include/structs.h
===================================================================
--- source/include/structs.h	(revision 5503)
+++ source/include/structs.h	(working copy)
@@ -153,6 +153,7 @@
 struct smb_composite_connect;
 struct smb_composite_sesssetup;
 struct smb_composite_fetchfile;
+struct smb_composite_appendacl;
 struct rpc_composite_userinfo;
 
 struct nbt_name;
Index: source/libcli/composite/composite.h
===================================================================
--- source/libcli/composite/composite.h	(revision 5536)
+++ source/libcli/composite/composite.h	(working copy)
@@ -140,3 +140,20 @@
 		uint16_t vuid;
 	} out;		
 };
+
+/*
+  composite call for appending new acl to the file's security descriptor and get 
+  new full acl
+*/
+
+struct smb_composite_appendacl {
+	struct {
+		const char *fname;
+
+		const struct security_descriptor *sd;
+	} in;
+	
+	struct {
+		struct security_descriptor *sd;
+	} out;
+};
Index: source/libcli/composite/appendacl.c
===================================================================
--- source/libcli/composite/appendacl.c	(revision 0)
+++ source/libcli/composite/appendacl.c	(revision 0)
@@ -0,0 +1,311 @@
+#include "includes.h"
+#include "libcli/raw/libcliraw.h"
+#include "libcli/composite/composite.h"
+#include "librpc/gen_ndr/ndr_security.h"
+
+/* the stages of this call */
+enum appendacl_stage {APPENDACL_OPENPATH, APPENDACL_GET, 
+		       APPENDACL_SET, APPENDACL_GETAGAIN, APPENDACL_CLOSEPATH};
+
+static void appendacl_handler(struct smbcli_request *req);
+
+struct appendacl_state {
+	enum appendacl_stage stage;
+	struct smb_composite_appendacl *io;
+
+	union smb_open *io_open;
+	union smb_setfileinfo *io_setfileinfo;
+	union smb_fileinfo *io_fileinfo;
+
+	struct smbcli_request *req;
+};
+
+
+static NTSTATUS appendacl_open(struct composite_context *c, 
+			      struct smb_composite_appendacl *io)
+{
+	struct appendacl_state *state = talloc_get_type(c->private, struct appendacl_state);
+	struct smbcli_tree *tree = state->req->tree;
+	NTSTATUS status;
+
+	status = smb_raw_open_recv(state->req, c, state->io_open);
+	NT_STATUS_NOT_OK_RETURN(status);
+
+	/* setup structures for getting fileinfo */
+	state->io_fileinfo = talloc(c, union smb_fileinfo);
+	NT_STATUS_HAVE_NO_MEMORY(state->io_fileinfo);
+	
+	state->io_fileinfo->query_secdesc.level = RAW_FILEINFO_SEC_DESC;
+	state->io_fileinfo->query_secdesc.in.fnum = state->io_open->ntcreatex.out.fnum;
+	state->io_fileinfo->query_secdesc.in.secinfo_flags = SECINFO_DACL;
+
+	state->req = smb_raw_fileinfo_send(tree, state->io_fileinfo);
+	NT_STATUS_HAVE_NO_MEMORY(state->req);
+
+	/* set the handler */
+	state->req->async.fn = appendacl_handler;
+	state->req->async.private = c;
+	state->stage = APPENDACL_GET;
+	
+	talloc_free (state->io_open);
+
+	return NT_STATUS_OK;
+}
+
+static NTSTATUS appendacl_get(struct composite_context *c, 
+			       struct smb_composite_appendacl *io)
+{
+	struct appendacl_state *state = talloc_get_type(c->private, struct appendacl_state);
+	struct smbcli_tree *tree = state->req->tree;
+	int i;
+	NTSTATUS status;
+	
+	status = smb_raw_fileinfo_recv(state->req, state->io_fileinfo, state->io_fileinfo);
+	NT_STATUS_NOT_OK_RETURN(status);
+	
+	/* setup structures for setting fileinfo */
+	state->io_setfileinfo = talloc(c, union smb_setfileinfo);
+	NT_STATUS_HAVE_NO_MEMORY(state->io_setfileinfo);
+	
+	state->io_setfileinfo->set_secdesc.level            = RAW_SFILEINFO_SEC_DESC;
+	state->io_setfileinfo->set_secdesc.file.fnum        = state->io_fileinfo->query_secdesc.in.fnum;
+	
+	state->io_setfileinfo->set_secdesc.in.secinfo_flags = SECINFO_DACL;
+	state->io_setfileinfo->set_secdesc.in.sd            = state->io_fileinfo->query_secdesc.out.sd;
+	talloc_steal(state->io_setfileinfo, state->io_setfileinfo->set_secdesc.in.sd);
+
+	/* append all aces from io->in.sd->dacl to new security descriptor */
+	if (io->in.sd->dacl != NULL) {
+		for (i = 0; i < io->in.sd->dacl->num_aces; i++) {
+			security_descriptor_dacl_add(state->io_setfileinfo->set_secdesc.in.sd,
+						     &(io->in.sd->dacl->aces[i]));
+		}
+	}
+
+	status = smb_raw_setfileinfo(tree, state->io_setfileinfo);
+	NT_STATUS_NOT_OK_RETURN(status);
+
+	state->req = smb_raw_setfileinfo_send(tree, state->io_setfileinfo);
+	NT_STATUS_HAVE_NO_MEMORY(state->req);
+
+	/* call handler when done setting new security descriptor on file */
+	state->req->async.fn = appendacl_handler;
+	state->req->async.private = c;
+	state->stage = APPENDACL_SET;
+
+	talloc_free (state->io_fileinfo);
+
+	return NT_STATUS_OK;
+}
+
+static NTSTATUS appendacl_set(struct composite_context *c, 
+			      struct smb_composite_appendacl *io)
+{
+	struct appendacl_state *state = talloc_get_type(c->private, struct appendacl_state);
+	struct smbcli_tree *tree = state->req->tree;
+	NTSTATUS status;
+
+	status = smbcli_request_simple_recv(state->req);
+	NT_STATUS_NOT_OK_RETURN(status);
+
+	/* setup structures for getting fileinfo */
+	state->io_fileinfo = talloc(c, union smb_fileinfo);
+	NT_STATUS_HAVE_NO_MEMORY(state->io_fileinfo);
+	
+
+	state->io_fileinfo->query_secdesc.level = RAW_FILEINFO_SEC_DESC;
+	state->io_fileinfo->query_secdesc.in.fnum = state->io_setfileinfo->set_secdesc.file.fnum;
+	state->io_fileinfo->query_secdesc.in.secinfo_flags = SECINFO_DACL;
+
+	state->req = smb_raw_fileinfo_send(tree, state->io_fileinfo);
+	NT_STATUS_HAVE_NO_MEMORY(state->req);
+
+	/* set the handler */
+	state->req->async.fn = appendacl_handler;
+	state->req->async.private = c;
+	state->stage = APPENDACL_GETAGAIN;
+	
+	talloc_free (state->io_setfileinfo);
+
+	return NT_STATUS_OK;
+}
+
+
+static NTSTATUS appendacl_getagain(struct composite_context *c, 
+				   struct smb_composite_appendacl *io)
+{
+	struct appendacl_state *state = talloc_get_type(c->private, struct appendacl_state);
+	struct smbcli_tree *tree = state->req->tree;
+	union smb_close *io_close;
+	NTSTATUS status;
+	
+	status = smb_raw_fileinfo_recv(state->req, c, state->io_fileinfo);
+	NT_STATUS_NOT_OK_RETURN(status);
+
+	io->out.sd = state->io_fileinfo->query_secdesc.out.sd;
+
+	/* setup structures for close */
+	io_close = talloc(c, union smb_close);
+	NT_STATUS_HAVE_NO_MEMORY(io_close);
+	
+	io_close->close.level = RAW_CLOSE_CLOSE;
+	io_close->close.in.fnum = state->io_fileinfo->query_secdesc.in.fnum;
+	io_close->close.in.write_time = 0;
+
+	state->req = smb_raw_close_send(tree, io_close);
+	NT_STATUS_HAVE_NO_MEMORY(state->req);
+
+	/* call the handler */
+	state->req->async.fn = appendacl_handler;
+	state->req->async.private = c;
+	state->stage = APPENDACL_CLOSEPATH;
+
+	talloc_free (state->io_fileinfo);
+
+	return NT_STATUS_OK;
+}
+
+
+
+static NTSTATUS appendacl_close(struct composite_context *c, 
+				struct smb_composite_appendacl *io)
+{
+	struct appendacl_state *state = talloc_get_type(c->private, struct appendacl_state);
+	NTSTATUS status;
+
+	status = smbcli_request_simple_recv(state->req);
+	NT_STATUS_NOT_OK_RETURN(status);
+	
+	c->state = SMBCLI_REQUEST_DONE;
+
+	return NT_STATUS_OK;
+}
+
+/*
+  handler for completion of a sub-request in appendacl
+*/
+static void appendacl_handler(struct smbcli_request *req)
+{
+	struct composite_context *c = req->async.private;
+	struct appendacl_state *state = talloc_get_type(c->private, struct appendacl_state);
+
+	/* when this handler is called, the stage indicates what
+	   call has just finished */
+	switch (state->stage) {
+	case APPENDACL_OPENPATH:
+		c->status = appendacl_open(c, state->io);
+		break;
+
+	case APPENDACL_GET:
+		c->status = appendacl_get(c, state->io);
+		break;
+
+	case APPENDACL_SET:
+		c->status = appendacl_set(c, state->io);
+		break;
+
+	case APPENDACL_GETAGAIN:
+		c->status = appendacl_getagain(c, state->io);
+		break;
+
+	case APPENDACL_CLOSEPATH:
+		c->status = appendacl_close(c, state->io);
+		break;
+	}
+
+	/* We should get here if c->state >= SMBCLI_REQUEST_DONE */
+	if (!NT_STATUS_IS_OK(c->status)) {
+		c->state = SMBCLI_REQUEST_ERROR;
+	}
+
+	if (c->state >= SMBCLI_REQUEST_DONE &&
+	    c->async.fn) {
+		c->async.fn(c);
+	}
+}
+
+
+/*
+  composite appendacl call - does an open followed by a number setfileinfo,
+  after that new acls are read with fileinfo, followed by a close
+*/
+struct composite_context *smb_composite_appendacl_send(struct smbcli_tree *tree, 
+							struct smb_composite_appendacl *io)
+{
+	struct composite_context *c;
+	struct appendacl_state *state;
+
+	c = talloc_zero(tree, struct composite_context);
+	if (c == NULL) goto failed;
+
+	state = talloc(c, struct appendacl_state);
+	if (state == NULL) goto failed;
+
+	state->io = io;
+
+	c->private = state;
+	c->state = SMBCLI_REQUEST_SEND;
+	c->event_ctx = tree->session->transport->socket->event.ctx;
+
+	/* setup structures for opening file */
+	state->io_open = talloc_zero(c, union smb_open);
+	if (state->io_open == NULL) goto failed;
+	
+	state->io_open->ntcreatex.level               = RAW_OPEN_NTCREATEX;
+	state->io_open->ntcreatex.in.root_fid = 0;
+	state->io_open->ntcreatex.in.flags            = 0;
+	state->io_open->ntcreatex.in.access_mask      = SEC_FLAG_MAXIMUM_ALLOWED;
+	state->io_open->ntcreatex.in.file_attr        = FILE_ATTRIBUTE_NORMAL;
+	state->io_open->ntcreatex.in.share_access     = NTCREATEX_SHARE_ACCESS_READ | NTCREATEX_SHARE_ACCESS_WRITE;
+	state->io_open->ntcreatex.in.open_disposition = NTCREATEX_DISP_OPEN;
+	state->io_open->ntcreatex.in.impersonation    = NTCREATEX_IMPERSONATION_ANONYMOUS;
+	state->io_open->ntcreatex.in.security_flags   = 0;
+	state->io_open->ntcreatex.in.fname            = io->in.fname;
+
+	/* send the open on its way */
+	state->req = smb_raw_open_send(tree, state->io_open);
+	if (state->req == NULL) goto failed;
+
+	/* setup the callback handler */
+	state->req->async.fn = appendacl_handler;
+	state->req->async.private = c;
+	state->stage = APPENDACL_OPENPATH;
+
+	return c;
+
+failed:
+	talloc_free(c);
+	return NULL;
+}
+
+
+/*
+  composite appendacl call - recv side
+*/
+NTSTATUS smb_composite_appendacl_recv(struct composite_context *c, TALLOC_CTX *mem_ctx)
+{
+	NTSTATUS status;
+
+	status = composite_wait(c);
+
+	if (NT_STATUS_IS_OK(status)) {
+		struct appendacl_state *state = talloc_get_type(c->private, struct appendacl_state);
+		state->io->out.sd = security_descriptor_copy (mem_ctx, state->io->out.sd);
+	}
+
+	talloc_free(c);
+	return status;
+}
+
+
+/*
+  composite appendacl call - sync interface
+*/
+NTSTATUS smb_composite_appendacl(struct smbcli_tree *tree, 
+				TALLOC_CTX *mem_ctx,
+				struct smb_composite_appendacl *io)
+{
+	struct composite_context *c = smb_composite_appendacl_send(tree, io);
+	return smb_composite_appendacl_recv(c, mem_ctx);
+}
+
Index: source/libcli/security/security_descriptor.c
===================================================================
--- source/libcli/security/security_descriptor.c	(revision 5503)
+++ source/libcli/security/security_descriptor.c	(working copy)
@@ -50,6 +50,46 @@
 	return sd;
 }
 
+static struct security_acl *security_acl_dup(TALLOC_CTX *mem_ctx,
+					     const struct security_acl *oacl)
+{
+	struct security_acl *nacl;
+	int i;
+
+	nacl = talloc (mem_ctx, struct security_acl);
+	if (nacl == NULL) {
+		return NULL;
+	}
+
+	nacl->aces = talloc_memdup (nacl, oacl->aces, sizeof(struct security_ace) * oacl->num_aces);
+	if ((nacl->aces == NULL) && (oacl->num_aces > 0)) {
+		goto failed;
+	}
+
+	/* remapping array in trustee dom_sid from old acl to new acl */
+
+	for (i = 0; i < oacl->num_aces; i++) {
+		nacl->aces[i].trustee.sub_auths = 
+			talloc_memdup(nacl->aces, nacl->aces[i].trustee.sub_auths,
+				      sizeof(uint32_t) * nacl->aces[i].trustee.num_auths);
+
+		if ((nacl->aces[i].trustee.sub_auths == NULL) && (nacl->aces[i].trustee.num_auths > 0)) {
+			goto failed;
+		}
+	}
+
+	nacl->revision = oacl->revision;
+	nacl->size = oacl->size;
+	nacl->num_aces = oacl->num_aces;
+	
+	return nacl;
+
+ failed:
+	talloc_free (nacl);
+	return NULL;
+	
+}
+
 /* 
    talloc and copy a security descriptor
  */
@@ -58,11 +98,45 @@
 {
 	struct security_descriptor *nsd;
 
-	/* FIXME */
-	DEBUG(1, ("security_descriptor_copy(): sorry unimplemented yet\n"));
-	nsd = NULL;
+	nsd = talloc_zero(mem_ctx, struct security_descriptor);
+	if (!nsd) {
+		return NULL;
+	}
 
+	if (osd->owner_sid) {
+		nsd->owner_sid = dom_sid_dup(nsd, osd->owner_sid);
+		if (nsd->owner_sid == NULL) {
+			goto failed;
+		}
+	}
+	
+	if (osd->group_sid) {
+		nsd->group_sid = dom_sid_dup(nsd, osd->group_sid);
+		if (nsd->group_sid == NULL) {
+			goto failed;
+		}
+	}
+
+	if (osd->sacl) {
+		nsd->sacl = security_acl_dup(nsd, osd->sacl);
+		if (nsd->sacl == NULL) {
+			goto failed;
+		}
+	}
+
+	if (osd->dacl) {
+		nsd->dacl = security_acl_dup(nsd, osd->dacl);
+		if (nsd->dacl == NULL) {
+			goto failed;
+		}
+	}
+
 	return nsd;
+
+ failed:
+	talloc_free(nsd);
+
+	return NULL;
 }
 
 /*
Index: source/libcli/config.mk
===================================================================
--- source/libcli/config.mk	(revision 5503)
+++ source/libcli/config.mk	(working copy)
@@ -22,7 +22,8 @@
 	libcli/composite/savefile.o \
 	libcli/composite/connect.o \
 	libcli/composite/sesssetup.o \
-	libcli/composite/fetchfile.o
+	libcli/composite/fetchfile.o \
+	libcli/composite/appendacl.o
 REQUIRED_SUBSYSTEMS = LIBCLI_COMPOSITE_BASE
 
 [SUBSYSTEM::LIBCLI_NBT]
Index: source/torture/raw/composite.c
===================================================================
--- source/torture/raw/composite.c	(revision 5503)
+++ source/torture/raw/composite.c	(working copy)
@@ -24,6 +24,7 @@
 #include "lib/events/events.h"
 #include "libcli/raw/libcliraw.h"
 #include "libcli/composite/composite.h"
+#include "librpc/gen_ndr/ndr_security.h"
 
 #define BASEDIR "\\composite"
 
@@ -109,7 +110,127 @@
 	return True;
 }
 
+
 /*
+  test setfileacl
+*/
+static BOOL test_appendacl(struct smbcli_state *cli, TALLOC_CTX *mem_ctx)
+{
+	struct smb_composite_appendacl **io;
+	struct smb_composite_appendacl **io_orig;
+	struct composite_context **c;
+
+	struct security_descriptor *test_sd;
+	struct security_ace *ace;
+	struct dom_sid *test_sid;
+
+	const int num_ops = 50;
+	char fname[50];
+	int *count = talloc_zero(mem_ctx, int);
+	struct smb_composite_savefile io1;
+
+	NTSTATUS status;
+	int i;
+
+	io_orig = talloc_array(mem_ctx, struct smb_composite_appendacl *, num_ops);
+
+	printf ("creating %d empty files and getting their acls with appendacl\n", num_ops);
+
+	for (i = 0; i < num_ops; i++) {
+		snprintf(fname, 50, BASEDIR "\\test%d.txt", i);
+		
+		io1.in.fname = fname;
+		io1.in.data  = NULL;
+		io1.in.size  = 0;
+	  
+		status = smb_composite_savefile(cli->tree, &io1);
+		if (!NT_STATUS_IS_OK(status)) {
+			printf("savefile failed: %s\n", nt_errstr(status));
+			return False;
+		}
+
+		io_orig[i] = talloc (io_orig, struct smb_composite_appendacl);
+		io_orig[i]->in.fname = talloc_strdup(io_orig[i], fname);
+		io_orig[i]->in.sd = security_descriptor_initialise(io_orig[i]);
+		status = smb_composite_appendacl(cli->tree, io_orig[i], io_orig[i]);
+		if (!NT_STATUS_IS_OK(status)) {
+			printf("appendacl failed: %s\n", nt_errstr(status));
+			return False;
+		}
+	}
+	
+
+	/* fill Security Descriptor with aces to be added */
+
+	test_sd = security_descriptor_initialise(mem_ctx);
+	test_sid = dom_sid_parse_talloc (mem_ctx, "S-1-5-32-1234-5432");
+
+	ace = talloc_zero(mem_ctx, struct security_ace);
+
+	ace->type = SEC_ACE_TYPE_ACCESS_ALLOWED;
+	ace->flags = 0;
+	ace->access_mask = SEC_STD_ALL;
+	ace->trustee = *test_sid;
+
+	status = security_descriptor_dacl_add(test_sd, ace);
+	if (!NT_STATUS_IS_OK(status)) {
+		printf("appendacl failed: %s\n", nt_errstr(status));
+		return False;
+	}
+
+	/* set parameters for appendacl async call */
+
+	printf("testing parallel appendacl with %d ops\n", num_ops);
+
+	c = talloc_array(mem_ctx, struct composite_context *, num_ops);
+	io = talloc_array(mem_ctx, struct  smb_composite_appendacl *, num_ops);
+
+	for (i=0; i < num_ops; i++) {
+		snprintf(fname, 50, BASEDIR "\\test%d.txt", i);
+
+		io[i] = talloc (io, struct smb_composite_appendacl);
+		io[i]->in.sd = test_sd;
+		io[i]->in.fname = talloc_strdup(io[i], fname);
+
+		c[i] = smb_composite_appendacl_send(cli->tree, io[i]);
+		c[i]->async.fn = loadfile_complete;
+		c[i]->async.private = count;
+	}
+
+	printf("waiting for completion\n");
+	while (*count != num_ops) {
+		event_loop_once(cli->transport->socket->event.ctx);
+		printf("count=%d\r", *count);
+		fflush(stdout);
+	}
+	printf("count=%d\n", *count);
+
+	for (i=0; i < num_ops; i++) {
+		struct security_descriptor sd;
+
+		status = smb_composite_appendacl_recv(c[i], io[i]);
+		if (!NT_STATUS_IS_OK(status)) {
+			printf("appendacl[%d] failed - %s\n", i, nt_errstr(status));
+			return False;
+		}
+		
+		security_descriptor_dacl_add(io_orig[i]->out.sd, ace);
+		if (!security_acl_equal(io_orig[i]->out.sd->dacl, io[i]->out.sd->dacl)) {
+			printf("appendacl[%d] failed - needed acl isn't set\n", i);
+			return False;
+		}
+	}
+	
+
+	talloc_free (ace);
+	talloc_free (test_sid);
+	talloc_free (test_sd);
+		
+	return True;
+}
+
+
+/*
   test a simple savefile/loadfile combination
 */
 static BOOL test_fetchfile(struct smbcli_state *cli, TALLOC_CTX *mem_ctx)
@@ -222,6 +343,7 @@
 
 	ret &= test_fetchfile(cli, mem_ctx);
 	ret &= test_loadfile(cli, mem_ctx);
+	ret &= test_appendacl(cli, mem_ctx);
 
 	smb_raw_exit(cli->session);
 	smbcli_deltree(cli->tree, BASEDIR);



More information about the samba-technical mailing list